Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite relative import extensions with flag #59767

Merged
merged 30 commits into from
Sep 27, 2024

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 26, 2024

This PR adds --rewriteRelativeImportExtensions to support transforming module specifiers from e.g. "./utils.ts" to "./utils.js". A module specifier will be statically rewritten or shimmed during JS emit if it meets all of these criteria:

  • it begins with ./ or ../
  • it does not end in a declaration file name (.d.ts, .d.mts, .d.cts, .d.*.ts)
  • it ends in .ts, .tsx, .mts, or .cts

Error checking

Errors are issued in an attempt to catch common mistakes:

  • A module specifier will get rewritten but will break after rewriting:
    • "./foo.ts" actually resolves to ./foo.ts.ts or foo.ts/index.ts
    • "../other-project/src/index.ts" belongs to another project where outputs go to a different outDir
  • A module specifier will not get rewritten but will break after emitting as-is:
    • "#blah/foo.ts" resolves to foo.ts where the .ts matches through a wildcard

Syntax support

Scenario Example input Operation Example output
Import declarations import "./foo.ts" Rewrite import "./foo.js", require("./foo.js")
Export declarations export * from "./foo.ts" Rewrite export * from "./foo.js"
Import equals import foo = require("./foo.ts") Rewrite import foo = require("./foo.js")
Import calls with qualifying literal argument import("./foo.ts") Rewrite import("./foo.js")
Require calls in JS with qualifying literal argument require("./foo.ts") Rewrite require("./foo.js")
Import calls with non-literal argument import(getPath()) Shim import(__rewriteRelativeImportExtension(getPath()))
Require calls in JS with non-literal argument require(getPath()) Shim require(__rewriteRelativeImportExtension(getPath()))
Import calls with non-qualifying literal argument import("foo") None import("foo")
Require calls with non-qualifying literal argument require("foo") None require("foo")
Require calls in TS require("./foo.ts") None require("./foo.ts")
Indirected require calls (require)("./foo.ts") None (require)("./foo.ts")

Limitations and future work

This PR doesn’t change how module resolution works, so it’s still possible to reference a .ts file by a .js module specifier:

// @Filename: a.ts
export {};

// @Filename: b.ts
import {} from "./a.js"; // Always ok

We discussed that it’s probably desirable to turn this functionality off under --rewriteRelativeImportExtensions (and --allowImportingTsExtensions), since these options are a strong indicator that the user is going to run the input TS files in a TS-native runtime like Bun or node --experimental-strip-types. That would be a breaking change and isn’t included in this PR.

tslib PR: microsoft/tslib#270
Fixes #59597

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 26, 2024
@andrewbranch
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 26, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 26, 2024

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/163457/artifacts?artifactName=tgz&fileId=2225118E5057943A4B80A3A9D8FE2EA5696226BA40FE09011C7C2241BF80844302&fileName=/typescript-5.7.0-insiders.20240826.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.7.0-pr-59767-2".;

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@andrewbranch andrewbranch changed the title [experiment] Rewrite relative import extensions with flag Rewrite relative import extensions with flag Sep 11, 2024
@microsoft microsoft deleted a comment from typescript-bot Sep 17, 2024
@bakkot
Copy link
Contributor

bakkot commented Sep 28, 2024

The original issue was locked; it would be nice for someone with permissions to comment there so that people who were following there get this update.

@jlmart88
Copy link

@RyanCavanaugh I happened upon your final post here: #49083 (comment) minutes before you commented and referenced this PR.

I'm curious what changed from your reasoning a year ago as to why this would never be a feature?

@RyanCavanaugh
Copy link
Member

NodeJS shipping --experimental-strip-types means there's now an actual runtime scenario where you need both JS and TS extensions

@RyanCavanaugh
Copy link
Member

Note that if you're using this in any other situation than that, you likely have a misconfiguration or misunderstanding of some kind, and I'll be telling you as much in a future issue when you report that e.g. this "doesn't work" across package boundaries.

@AlttiRi
Copy link

AlttiRi commented Oct 2, 2024

I have tested TS 5.7.0-dev.

It does not rewrite .d.ts files.

My library preset on 5.6.2:

    /* Library preset */
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "noEmit": false,
    "declaration": true,

and new one on 5.7.0-dev:

    /* Library preset */
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "allowImportingTsExtensions": true,      // <- new
    "rewriteRelativeImportExtensions": true, // <- new
    "noEmit": false,
    "declaration": true,

It works, I can use tsc (after changing .js to .ts in imports in all files).

Also, now I can run TS files with deno without using --unstable-sloppy-imports which prints annoying warnings.

However, imports inside .d.ts files have .ts ending now. There were no rewriting.

Or is it OK? (I didn't check if there would be .d.ts files to work correctly after installing a package with such .d.ts files.)

Anyway, I think that both configs should produce absolutely the same result (.js and .d.ts files) after using tsc.

@RyanCavanaugh
Copy link
Member

Imports in .d.ts files are allowed to use .ts extensions regardless of config, so I wouldn't really consider that a bug.

@AlttiRi
Copy link

AlttiRi commented Oct 2, 2024

It's a bug. Non-rewritten imports in d.ts. break (partially) the code assistance in an IDE.

It's OK:

Screenshot
(npm install @alttiri/util-js@1.14.0-20240821)

After using rewriteRelativeImportExtensions:

Screenshot_1
(npm install @alttiri/util-js@1.15.0-dev.20241002 --registry=https://npm.pkg.github.com)

In this case, the IDE no longer highlights the function names, nor does it show the function signature.

The imports in .d.ts also must be rewritten.

Because all d.ts files of libs are compiled with tsc have imports end with .js, never with .ts.

Because allowImportingTsExtensions was never compatible with "moduleResolution": "NodeNext" + "noEmit": false" (until this new rewriteRelativeImportExtensions appeared.)

@AlttiRi
Copy link

AlttiRi commented Oct 3, 2024

Also, it currently (?) does not support aliases which resolve to a relative path.

{
  "extends": "../tsconfig.json",
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "@/*": ["../*"]
    }
  }
}

I get the follow error for import {sleep} from "@/index.ts";:

TS2877: This import uses a '.ts' extension to resolve to an input TypeScript file, but will not be rewritten during emit because it is not a relative path.

@RyanCavanaugh
Copy link
Member

Also, it currently (?) does not support aliases which resolve to a relative path.

This is 100% intentional

@AlttiRi
Copy link

AlttiRi commented Oct 4, 2024

Is it a problem to resolve the alias first and only after that check if it is a relative path or not?

It doesn't seem like something that is difficult to implement. Just only one extra simple action.

@RyanCavanaugh
Copy link
Member

I don't think the setup you have there works under --experimental-strip-types. #59767 (comment)

@AlttiRi
Copy link

AlttiRi commented Oct 4, 2024

Okay, I have checked it with the latest Node.js (v22.9.0) too (I tested it with Deno before).

It works (as expected).

Screenshot

@RyanCavanaugh
Copy link
Member

I can't replicate what you're doing there

// with
// import * as bar from "./bar.ts";

node --experimental-strip-types foo.ts
hi

vs

// with
// import * as bar from "@/bar.ts";
Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@/bar.ts' imported from D:\Throwaway\est-test\foo.ts

The purpose of the flag is to rewrite paths which are syntactically relative, not "semantically" relative (whatever that would even mean).

@nicolo-ribaudo
Copy link

Hey, I was reading the table in the PR description and I'm getting confused by these two lines:

Scenario Example input Operation Example output
Require calls in JS with qualifying literal argument require("./foo.ts") Rewrite require("./foo.js")
Require calls in TS require("./foo.ts") None require("./foo.ts")

Does it mean that require("./foo.ts") will be rewritten if it's inside a JS file, but not if it's inside a TS file?

@andrewbranch
Copy link
Member Author

Yes. Plain require calls aren’t recognized as special in TS files. You’ll notice in JS that require("./foo") performs module resolution (the compiler knows the type to return), but in TS, require isn’t even allowed unless it’s declared as a global function, in which case that function’s return type is used (usually any).

jpwilliams added a commit to inngest/inngest-js that referenced this pull request Nov 25, 2024
jpwilliams added a commit to inngest/inngest-js that referenced this pull request Nov 25, 2024
## Summary
<!-- Succinctly describe your change, providing context, what you've
changed, and why. -->

Ensures support for `typescript@~5.7.0` by adding a new PR check. See
[Announcing TypeScript 5.7 -
TypeScript](https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/).

Ideally we would take advantage of the new
`rewriteRelativeImportExtensions` added in microsoft/TypeScript#59767
which would allow us to correctly import `.ts` extensions instead of
requiring `.js`, ready for better Deno/Bun support, as well as Node
support when running `.ts` files directly.

Unfortunately, `typescript<5.0.0` doesn't support the compiled output;
we'll have to wait for v4 to use that.

## Checklist
<!-- Tick these items off as you progress. -->
<!-- If an item isn't applicable, ideally please strikeout the item by
wrapping it in "~~"" and suffix it with "N/A My reason for skipping
this." -->
<!-- e.g. "- [ ] ~~Added tests~~ N/A Only touches docs" -->

- [ ] ~Added a [docs PR](https://github.com/inngest/website) that
references this PR~ N/A KTLO
- [x] Added unit/integration tests
- [x] Added changesets if applicable

## Related

- https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/
- microsoft/TypeScript#59767
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite imports with ".ts" extension to ".js" to support Nodejs's "--experimental-strip-types"
10 participants