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

[WIP] ESM support for node v13 #1010

Merged
merged 17 commits into from
May 3, 2020
Merged

[WIP] ESM support for node v13 #1010

merged 17 commits into from
May 3, 2020

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Apr 13, 2020

I started implementing ESM support, because it's an interesting problem and I wanted to see if I could get it working in a day.

Implements #1007
Related: #935

Example

Checkout the branch, build the code, and look at ./esm-experiment/README.md

Roughly, usage is node --loader ts-node/esm ./index.js

Try it on your own project

npm lets you install a dependency directly from a git branch.

npm install --save TypeStrong/ts-node#ab/esm-support

Limitations

compileEsm falls back to ts.transpileModule when the regular compile code-path would otherwise emit CommonJS. When this happens, no typechecking. If user sets module: "esnext" in their tsconfig, then this is not a problem and we use the main codepath to emit ESM.

sourceMapSupport will break if the same file is loaded as both ESM and CJS, because our outputCache can't have 2 emits for the same filename.
We avoid this by detecting when a file is being loaded by both ESM and CJS, and throwing an error.
In practice, I can't think of a reason why the same .ts file would be loaded as both ESM and CJS, so this should never throw. If it does, the error message will show up in our issue tracker.

Development work

  • implementing compileEsm()
  • make sure transpileModule call looks correct
  • fix return type of compileEsm; it doesn't match compile
  • compile fileName in transformSource() hook
  • implement resolution lookup in resolve() hook
    • impl is incomplete (see other bullet points)
  • Make resolution lookup use our fs caches
  • Make resolution lookup replace .js with .ts as needed
  • Create esm-script.mjs to do --script-mode?
    • Can read process.argv for config resolution
  • automated tests

Misc

resolve() logic

We need to add additional file extensions to ESM's module resolution behavior. We also need to automatically add extensions when they're missing, and check if we should rewrite .js into .ts.
I've done this by copy-pasting a copy of node's source code, patched to tweak the behavior. This approach looks messy when you see it in our source tree, but it's a great way to match node's behavior without much complexity.

Ideally this functionality is maintained as a third-party dependency. So far, the only similar library I'm aware of is this one, used by webpack:
https://www.npmjs.com/package/enhanced-resolve
It implements require.resolve, but it's async and very configurable.

@cspotcode cspotcode marked this pull request as draft April 13, 2020 21:47
@evg656e
Copy link

evg656e commented Apr 23, 2020

Looks like esm.mjs missing on #ab/esm-support branch (should be on 'files' sections i suppose). And resolution still fails when modules internally rely on '--experimental-specifier-resolution=node' flag.

@cspotcode
Copy link
Collaborator Author

@evg656e Good catch; I added the missing entry to our files array.

How do you propose we support --experimental-specifier-resolution=node? Remember that we need to register our own resolver hook. It's also unclear whether or not modules will be allowed to rely on experimental-specifier-resolution=node. It's experimental. It might be removed. Certainly third-party code wouldn't be allowed to rely on it, because the consumer might not have it enabled.

@evg656e
Copy link

evg656e commented Apr 23, 2020

@cspotcode Is it flag experimental or the resolution itself? As far as i concerned, this resolution algorithm (sort of) is used by typescript itself when it comes to module loading. And i don't see much utility in esm modules in node without it.

Can we inspect node options to see, whether this flag was used or not? And switch to appropriate algorithm?

@cspotcode
Copy link
Collaborator Author

cspotcode commented Apr 23, 2020

@evg656e I think it would help if you share a specific example of what you want to achieve, and how it is not working currently.

ESM hooks are experimental. This includes experimental-specifier-resolution=node and --loader / --experimental-loader. For example, in testing, I was able to make node reliably segfault, so it's got some serious bugs.

When TypeScript is set to "moduleResolution": "node", that is the CommonJS style of node resolution, not necessarily the new ESM resolution. ts-node still works well with CommonJS. You can take a look at our resolver; it adds missing file extensions, for example, and it resolves node_modules.

Can we inspect node options to see, whether this flag was used or not? And switch to appropriate algorithm?

If you find out, let us know. You'll probably need to ask the relevant people on the node team, since the hooks feature is still in flux and they are changing things.

@evg656e
Copy link

evg656e commented Apr 23, 2020

@cspotcode I've created sample repo: https://github.com/evg656e/rebenchmark-example. I've also forked this branch and add some changes to make my example work (it did not work with the explicit resolution algorithm).

I also briefly looked at whether it's possible to somehow access the node options through process.argv inside loader, and it seems like it's impossible (options like --loader or --experimental-specifier-resolution disappear)

@cspotcode
Copy link
Collaborator Author

@evg656e excellent, thank you. Here's a link to your branch, for anyone else who might be reading this: https://github.com/evg656e/ts-node/tree/ab/esm-support

I probably will not have time to look at this till the weekend, but I appreciate the help.

@evg656e
Copy link

evg656e commented Apr 24, 2020

@cspotcode Actually, there is process.execArgv that allows to inspect Node invocation options. I modified getOptionValue code, now it respects passed options.

Last question is why there no '.js' extension in extensions variable. According to Node sources (https://github.com/nodejs/node/blob/d01a06a916efd30844e1e0a38e79dc0054fc4451/lib/internal/modules/esm/resolve.js#L240) it should be there, and it causes my example not to work also.

@cspotcode
Copy link
Collaborator Author

@evg656e Cool beans, I have a few questions about the resolve behavior, to make sure we end up with a solid implementation:

  • I'm concerned that parsing process.execArgv may be a hack that's prone to failure. Do we have to parse multiple variants of that flag being passed, to ensure we behave identically to node's parsing? (--flag=value, --flag value, and how is weird stuff like --require --flag parsed?)
  • We're installing a hook anyway; is it easier for us to implement our own flag? The idiomatic ts-node way of doing this is via an env var.
  • Are there situations where an end-user must use node's built-in-flag verbatim, since they're already going to be passing --loader ts-node/esm?
  • What if we allowed the user to pass --loader ts-node/esm-node? Are there situations where our hook is installed, but the default resolve behavior is still being invoked and needs to be preserved?

The missing .js extension is probably an oversight. It's similar, though not identical, to one of the TODO items in my original post: #1010 (comment) We need a way to resolve to .js files but recognize when we're not supposed to be compiling them.


What is the intention of your example? I see it running a benchmark tool, and there's only a single .ts file. I think it would help if we have a clearer idea of what the end-user is trying to accomplish in that example, and why ESM support is necessary.

@evg656e
Copy link

evg656e commented Apr 24, 2020

@cspotcode The idea of this example that i have a benchmark tool, that internally uses esm modules (even with 3rdparty non-esm modules). And it works fine with experimental-specifier-resolution=node flag. I've tried to implement 'out of the box' typescript support. And currently it's impossible to do that with cjs ts-node/register. Then i found your proposal, and with a bit tweaks i was able to make it work. Demo repo has only 2 benchmarks (one js and one ts), but of course end user will be able to add any number of benchmarks.

More over, i'm experimenting with esm modules in node for a while, and have other projects that rely on this flag. I suppose, that without this flag any legacy cjs module will not work in Node. As i can see, this flag actively maintained for now (there is bug in resolve algorihtm actively being worked on). So i don't think ignoring this flag at all is a good idea.

I'm concerned that parsing process.execArgv may be a hack that's prone to failure

I used npm arg module to parse process.execArgv, ts-node have this parser in dependencies, so i think it will work fine (i have tested trivial use cases like omiting '=' sign). Of course, it may not be 100% node compatible, since we have no access to internal getOption function.

The idiomatic ts-node way of doing this is via an env var.

I have thought about this too, but since i found process.execArgv i decided to use this option. Theoretically we can have both approaches, one fallback for another. Need more feedback.

Mocha recently added native esm support too, so i think we can look for their implementation decisions.

One more thought: I get an error even when I have not loaded a single ts file. This means that the current custom loader applies its internal resolution algorithm even for js files, although logically, it should fallback to the default implementation (defaultResolve, like mentioned in docs https://nodejs.org/api/esm.html#esm_transpiler_loader).

@cspotcode
Copy link
Collaborator Author

@evg656e if I understand correctly, you want to let users write their benchmark files in ESM? If so, can we update the example so that the benchmark files are loaded as ESM? Right now the package.json doesn't specify, so they're defaulting to CommonJS. Your benchmark tool could require() them and everything would work.

Even without experimental-specifier-resolution=node, legacy modules still work because they are loaded via the CommonJS algorithms, not the new ESM resolver, right?

One more thought: I get an error even when I have not loaded a single ts file.

Please post a copy of errors when you encounter them; it helps everyone debug.

This means that the current custom loader applies its internal resolution algorithm even for js files

Yeah, sounds like the same oversight as mentioned here #1010 (comment)
However, we do want to allow non-compiled .js files to load .ts files.


I noticed your benchmark tool doesn't work on Linux; is that intentional?
https://github.com/evg656e/rebenchmark/blob/master/bin/rebenchmark.js#L1

@evg656e
Copy link

evg656e commented Apr 24, 2020

Right now the package.json doesn't specify, so they're defaulting to CommonJS.

I think i tried both variants and all variants failed with your initial custom loader. Over more, it doesn't matter at all when i don't use custom loader (both variants work). I think it work because tool itself has proper package.json with specified type: 'module'.

Your benchmark tool could require() them and everything would work.

Internally it uses dynamic import. It's by design. There is similar bench-runner build upon cjs modules.

I noticed your benchmark tool doesn't work on Linux; is that intentional?

It's a bug, i have never tested this in Linux, copied this line from stackoverflow.

Even without experimental-specifier-resolution=node, legacy modules still work because they are loaded via the CommonJS algorithms, not the new ESM resolver, right?

Not sure. I think if you importing some CJS module from ESM module, e.g. somwhere in your code: import foo from 'bar', and bar is CJS module, then first will be invoked ESM resolver. And without experimental-specifier-resolution=node flag it will definitely will fail. But inside bar CJS resolver will be used.

I can create some abstract repo with corner cases if you wish.

Please post a copy of errors when you encounter them; it helps everyone debug.

Error: ERR_MODULE_NOT_FOUND C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\rebenchmark\lib\reporters C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\rebenchmark\lib\cli.js module at finalizeResolution (C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist-raw\node-esm-resolve-implementation.js:329:11) at moduleResolve (C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist-raw\node-esm-resolve-implementation.js:677:10) at Object.defaultResolve (C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist-raw\node-esm-resolve-implementation.js:718:13) at C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\src\esm.ts:50:38 at Generator.next (<anonymous>) at C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist\esm.js:8:71 at new Promise (<anonymous>) at __awaiter (C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist\esm.js:4:12) at resolve (C:\Users\fromh\Projects\sandbox\github\rebenchmark-example\node_modules\ts-node\dist\esm.js:29:16) at Loader.resolve (internal/modules/esm/loader.js:97:40)

An error occurs on import '../lib/cli'; from rebenchmark/bin/rebenchmark.js entry point.

@cspotcode
Copy link
Collaborator Author

Linux does not allow passing arguments via shebangs. This is controlled by the Linux kernel. We have a bunch of other tickets where this has caught people off-guard, but to clarify, it's not a ts-node or a shell issue. Since it may affect developer experience, do you know how you want to support Linux? I can think of a few ways you can do it, but it will help to know which you prefer.

I think if you importing some CJS module from ESM module, e.g. somwhere in your code: import foo from 'bar', and bar is CJS module, then first will be invoked ESM resolver. And without experimental-specifier-resolution=node flag it will definitely will fail.

Actually this should succeed without experimental-specifier-resolution=node. Resolving bare module identifiers is supported in the explicit resolver, too. https://nodejs.org/api/esm.html#esm_features

When you don't use our ESM hook, the benchmark files load as CommonJS. When you do use our ESM hook, the missing .js extension means the resolver doesn't see benchmarks/string.js, so it fails. Once we add that missing .js extension, the benchmark files will be resolved and loaded as CommonJS. You'll still need the ESM hook for resolution, but compilation of CommonJS will happen via our old CommonJS codepath.

@cspotcode
Copy link
Collaborator Author

@evg656e I added the missing extension. Good catch, thanks.

Our transformSource hook calls ignored, which should make sure we don't compile .js unless allowJs is enabled.
https://github.com/TypeStrong/ts-node/blob/ab/esm-support/src/esm.ts#L99-L101
https://github.com/TypeStrong/ts-node/blob/ab/esm-support/src/index.ts#L806-L814

@evg656e
Copy link

evg656e commented Apr 24, 2020

Actually this should succeed without experimental-specifier-resolution=node

Yep, you are right. Problem with my projects that import statements does not use explicit extension. Since then i need to set flag.

@cspotcode
Copy link
Collaborator Author

cspotcode commented Apr 24, 2020

@evg656e Ok, that's good. Our ESM loader should be adding missing file extensions, so this should automatically work for you without the ...-resolution=node flag:
https://github.com/TypeStrong/ts-node/blob/ab/esm-support/dist-raw/node-esm-resolve-implementation.js#L310

I explained why we need to auto-add missing file extensions in the proposal: #1007
In short, TS auto-imports omit the file extension, so we need to auto-add it for a good developer experience. (If you hit "add import" in your editor, the resulting code should work)

@evg656e
Copy link

evg656e commented Apr 29, 2020

I tried to use your loader with mocha (recent versions of mocha support native ESM modules) and came across with a new bug:

node --loader ts-node/esm node_modules/mocha/bin/mocha

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\mocha\bin\mocha
    at defaultGetFormat (internal/modules/esm/get_format.js:65:15)
    at defer (C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\ts-node\src\esm.ts:57:33)
    at C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\ts-node\src\esm.ts:79:18
    at Generator.next (<anonymous>)
    at C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\ts-node\dist\esm.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\ts-node\dist\esm.js:4:12)
    at getFormat (C:\Users\fromh\Projects\sandbox\github\ts-node\esm-usage-example\mocha\node_modules\ts-node\src\esm.ts:55:5)
    at Loader.getFormat (internal/modules/esm/loader.js:113:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:244:31)

Without a loader, everything works fine. I added an example to my branch: https://github.com/evg656e/ts-node/tree/ab/esm-support/esm-usage-example/mocha

@cspotcode
Copy link
Collaborator Author

@evg656e Looks like this is caused by node v14 and not related to ts-node. Here's a reproduction that does not use ts-node.

touch empty-loader.mjs # <-- loader hooks that do not do anything
touch bar # <-- extensionless javascript file
node ./bar # <-- works correctly
node --loader ./empty-loader.mjs ./bar # <-- throws the error you are seeing

If you think it's a bug, you can file a ticket with node. Otherwise, mocha may need an update to be compatible with loaders.

…ort for experimental-specifier-resolution=node
@cspotcode
Copy link
Collaborator Author

The commit above is untested but it implements the following:

  • resolves foo.js to foo.ts, foo.tsx, or foo.jsx, to match what TypeScript does and support including the .js extension in module specifiers

  • remove the compileEsm codepath. Everything is compiled the same. If you want to use ESM, you need to set your tsconfig accordingly. This means the ESM codepath consistently gets typechecking; much simpler and without caveats

  • merges @evg656e 's execArgv parsing so that we support --experimental-specifier-resolution=node

@coveralls
Copy link

coveralls commented Apr 30, 2020

Coverage Status

Coverage increased (+0.6%) to 80.709% when pulling 0ea6007 on ab/esm-support into 8372d4d on master.

@cspotcode cspotcode marked this pull request as ready for review May 3, 2020 00:22
@cspotcode
Copy link
Collaborator Author

Added some tests; fixed the tests. I'll merge and publish tonight.

@cspotcode cspotcode merged commit f6cd5d4 into master May 3, 2020
@cspotcode
Copy link
Collaborator Author

This is released as an experimental feature in v8.10.0. Please test and share your feedback in #1007.

@cspotcode cspotcode deleted the ab/esm-support branch May 17, 2020 00:54
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.

3 participants