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

feat(register): resolve .js to .ts in ts-node/register #1361

Closed
wants to merge 4 commits into from

Conversation

calebboyd
Copy link

@calebboyd calebboyd commented Jun 3, 2021

Fixes #783

Makes A work #783 (comment)

Adds an entrypoint ts-node/register/try-ts-ext That applies a patch to attempt resolution of a ts file when a js file returns MODULE_NOT_FOUND

Reasoning:

Ran into this when trying to do dual mode modules, that is developing with tooling set to commonjs, and shipping both esm and commonjs.

@calebboyd calebboyd force-pushed the resolve-filename branch 3 times, most recently from 776d768 to 9c4b462 Compare June 3, 2021 17:19
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #1361 (17aad13) into main (17aad13) will not change coverage.
The diff coverage is n/a.

❗ Current head 17aad13 differs from pull request most recent head b630c75. Consider uploading reports for the commit b630c75 to get more accurate results

@calebboyd calebboyd changed the title feat(register): swap js -> ts on MODULE_NOT_FOUND feat(register): add try-ts-ext register entrypoint Jun 3, 2021
@cspotcode
Copy link
Collaborator

cspotcode commented Jun 4, 2021

Thanks for sending this, I almost forgot to respond. (well, I did forget yesterday) This is a pretty important feature; really glad to support this.

I vaguely remember when I was implementing this .js -> .ts mapping for our ESM resolver, there were corner cases that prevented me from taking the approach being used here: try .js, catch error, try .ts

Due to the complexity of node's module resolution algorithm, and the fact that it checks many directories and package.json files, I think there are rare cases where it can actually resolve to a .js file when it's supposed to resolve to a .ts file in an entirely different directory -- a directory that it had checked several steps prior in the resolution algorithm.

We also have a preferTsExts option which is meant to tell ts-node to load a .ts file even if there's already a presumably precompiled .js file. In those cases, behavior would have to be slightly different.

For the ESM resolver, we ended up copying node's resolver and patching it to do what we needed.
https://github.com/TypeStrong/ts-node/compare/esm-resolver-diff..main#diff-8d22ea0b9996d7aafc28297d78c308bd2dc5028b03ab0dc701d2849056b958f1R329-R347

We could probably do the same for the CommonJS resolver.


For option naming, would the name change if we also added mapping from outDir to rootDir -- ./dist/ to ./src/? I believe this kind of mapping is necessary for certain monorepo scenarios. Should we name the option something like commonjsResolverHook or is that too confusing?

@calebboyd
Copy link
Author

I agree it's an important feature especially right now. This quick implementation is really naive as you mentioned, and solves the problem reliably only when outDir is in a different tree entirely. Lack of outDir, has node potentially finding stale built js.

Due to the complexity of node's module resolution algorithm

Do these complexities still exist if we have a relative request with an exact extension?

We also have a preferTsExts option

I'm curious on your thoughts as to whether this behavior should roll up into the preferTsExts option, or imply it. Another approach here might be to drop js extensions entirely from requests, and then defer to preferTsExts and the existing node resolution.

Should we name the option something like commonjsResolverHook or is that too confusing?

I'm not sure if we should blend/burden commonjs extensions api with the esm loader terminology.

@calebboyd
Copy link
Author

calebboyd commented Jun 5, 2021

I tried the other approach here and it seems to do the trick. And I can't immediately think of any corner cases this wouldn't work

Still need to do:

  • don't drop explicit ext from non-relative specifiers.
  • don't drop ext if parent is prefixed by an entry in 'module' paths

calebboyd@ce8ad2c

npm i https://github.com/calebboyd/ts-node.git#rfd

@cspotcode
Copy link
Collaborator

I'm not sure if we should blend/burden commonjs extensions api with the esm loader terminology.

It feels like the same terminology should be used for both, right? Because we are, indeed, hooking resolution behavior. require.extensions is the transform hook, and require._resolveFilename is the resolve hook. I want to pick an option that's future-proof and meets the user's expectations. But it's tough because adding to require.extensions also affects resolution, so we already kinda sorta have a resolve hook.

I'm also thinking this CJS resolve hook can become the default in a future release.

I'm curious on your thoughts as to whether this behavior should roll up into the preferTsExts option, or imply it.

I think this depends on the project. The way I see it, if a .js and .ts file both exist, they are considered to be one and the same "virtual file," and the question is which we should execute. In a perfect future ts-node, these files could even be in separate directories, ./dist/index.js and ./src/index.tsx. preferTsExts is the user telling us "always use the source even if something has been precompiled."

This is distinct from our scope option (#1346) which tells ts-node to limit itself to compiling only within a single directory, for example ./my-monorepo/packages/only-compile-this-package

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 5, 2021

Now that I think about it, if the user has allowJs enabled, then a "virtual file" might exist at both ./dist/index.js and ./src/index.js. In those cases, a better name than preferTsExts would be preferSourceFiles. I realize that's getting into some other future changes that you probably don't want to spend your time hashing out.

@cspotcode
Copy link
Collaborator

I've jotted down some funky CJS resolver situations: https://github.com/TypeStrong/ts-node-repros/tree/commonjs-resolver-corner-cases

@calebboyd
Copy link
Author

calebboyd commented Jun 6, 2021

It feels like the same terminology should be used for both, right?

Yeah, that makes sense, sgtm.

Is there anyway used to determine the scope from active tsconfigs? (isSourceFile?)..

Relative specifiers of mono-repo (workspace) packages seems strange to me... Typically those would be linked from node_modules, right?

Playing around with the corner cases I ended up here, but definitely need a way to resolve the "virtual file" you're talking about.

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 8, 2021

Is there anyway used to determine the scope from active tsconfigs? (isSourceFile?)..

I'm not sure I understand this question. Is this referring specifically to the scope / scopeDir options?

@calebboyd
Copy link
Author

calebboyd commented Jun 10, 2021

I'm not sure I understand this question. Is this referring specifically to the scope / scopeDir options?

@cspotcode I wasn't familiar with those options. I'm more curious if we can determine if a resolved filename is a sourcefile. But even that might be too in the weeds...

I think the problem that needs solving could be reduced to "do what tsc does", which in this case, it treats relative imports of .js specifiers as .ts files first, and the same for a directory + package#main file. (see pr here)

I think until typescript supports more complicated resolutions via exports, it probably doesn't need to be more complicated than that? But I'll defer to your judgement.

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 10, 2021

The SourceFile idea probably won't work in transpileOnly mode, since we don't have a proper Program in that mode; we call ts.transpileModule or swc.transformSync. We do parse the tsconfig using full, proper TypeScript config parsing, so maybe that is enough to give us this information?

Sometimes tsc resolves to a .d.ts and node resolves to a .js file. And tsc obeys package.json typesVersion whereas node looks at exports / main. Not sure if those are risks; just thinking out loud.

Perhaps the trickiest thing for now is: when node_modules/mylibrary/package.json says "main": "lib/foo.js" we need to resolve import "mylibrary" to ./node_modules/mylibrary/lib/foo.ts. And we cannot accidentally resolve to ../node_modules/mylibrary/lib/foo.js (not sure if that's a risk or not)

In that scenario, ./node_modules/mylibrary might actually be a symlink to ../mylibrary in a monorepo situation.

I think if we can get that, we should be in a pretty good place.

@calebboyd calebboyd force-pushed the resolve-filename branch 3 times, most recently from f88eb40 to dd9044b Compare August 21, 2021 12:57
@calebboyd calebboyd changed the title feat(register): add try-ts-ext register entrypoint feat(register): resolve .js to .ts in ts-node/register Aug 21, 2021
achingbrain added a commit to achingbrain/ip-address that referenced this pull request Nov 23, 2021
This fixes two problems with the ESM build of this module.

1. The `package.json` that contains `{ "type": "module" }` wasn't being included in the npm tarball
2. When running in an ESM environment, `import foo from './bar'` does not work, you have to specify the extension

The fix for the first is simple, add the cjs/esm `package.json` files to the `files`
array in the project `package.json`.

The second fix is harder.  If you just add the `.js` extension to the source files,
typescript is happy but ts-node is not, and this project uses ts-node to run the
tests without a compile step.

Typescript does not support importing `*.ts` and will not support adding `*.js` to
the transpiled output - microsoft/TypeScript#16577

ts-node thought this was a bug in Typescript but it turns out not.  Their suggestion
to use `ts-node/esm` breaks sourcemap support because `source-map-support/register`
is not esm - TypeStrong/ts-node#783

There is a PR against ts-node to add support for resolving `./foo.js` if `./foo.ts`
or `./foo` fails but it seems to have stalled - TypeStrong/ts-node#1361

Given all of the above, the most expedient way forward seemed to just be to add
a shell script that rewrites the various `import` statements in the esm output
to add the `.js` extension, then if the ts-node PR ever gets merged the script
can be backed out.

Fixes beaugunderson#147
@achingbrain
Copy link

Applying this patch locally seems to work well enough that I can use ts-node to run tests with mocha without needing an explicit compilation step (yay!) along with source maps and correct stack traces etc.

This makes ts much easier to work with, is there any chance this could get merged?

@calebboyd
Copy link
Author

Unfortunately, while this does work, (I also use this branch) I think the full solution will need to account for all the scenarios outlined in the epic #1514. A concise and pragmatic solution for that isn't straight forward.

@cspotcode
Copy link
Collaborator

cspotcode commented Jan 27, 2022

I'm revisiting this. Although a complete resolver is still ideal, and I want to get there eventually, I also want to fix the simpler issue of mapping .js to .ts in local, relative imports. Looking back, I was probably being too idealistic in hoping to implement a complete resolver all at once.

I'm thinking I'll keep it behind an experimental* flag until the full resolver is implemented. I know experimental flags can seem messy, but it really reduces my stress levels around getting the full resolver implemented while still unblocking users who require this feature today. Hopefully it is no great hardship for users to add "experimentalCjsResolver": true to their tsconfig file. But if anyone has a use-case which makes this unreasonable, I would definitely appreciate a ticket explaining their situation.

Merging this in some form will unblock #1564. Will also need to update it to handle .cjs and .mjs from #1564. And will need to be merged with the path-mapping hook from #1585.

@calebboyd
Copy link
Author

I completely understand 😅 . I'll see if I can get this updated in the near future to account for the things you mentioned + tests.

@cspotcode
Copy link
Collaborator

Much appreciated, usually when I leave a PR stuck in review for this long, the author abandons me. Can't say I blame them either. :)

I made some code changes locally, mostly replacing the tryTsExts flag with a experimentalResolverFeatures flag. I'll try to push those today or this weekend.

I also spent some time last night revisiting the "complete resolver" idea so I would have a better idea of the path forward, beyond this PR. The problem isn't how to do the mappings, the question is when. I think "when" is determined both by the parent's location and the resolved target's location. (In a monorepo, if a non-TS package tries to require() into a TS package, do the mappings. stuff like that) It's sorta out-of-scope for this PR but I might be posting some of that stuff here as I try to wrap my head around all the possibilities.

@cspotcode
Copy link
Collaborator

cspotcode commented Jan 31, 2022

I've merged #1614 into this PR, meant to establish a common core for _resolveFilename hooks. In particular, I hope it avoids a painful merge with #1585. So all my work-in-progress code changes are now pushed.

@cspotcode
Copy link
Collaborator

I have finally (!) implemented a complete commonjs resolver that can do these file extension re-mappings: #1727

This is happening alongside #1694, which adds full support for the new node support, with cts, mts, cjs, mjs extensions.

Hoping to get this all released ahead of TS 4.7.0.

Thanks again for sending this PR; closing it as I ended up going another route, but I appreciate it all the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find module when explicit import extension Support .js extensions.
3 participants