-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
module: improve external format support #49704
module: improve external format support #49704
Conversation
lib/internal/modules/esm/loader.js
Outdated
const wellKnownCommunityLoaders = { | ||
'__proto__': null, | ||
'typescript': [ | ||
'ts-node', | ||
'sucrase', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before shipping this, we need some kind of policy for how to define what goes into this list. That could maybe be part of the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, I think the error message should just link to something in https://nodejs.org/en/docs/guides. Then we can have a longer list with detailed instructions. It also protects us against one of these recommendations becoming outdated, like if one of the packages in this list goes unmaintained, we don’t want old versions of Node continuing to recommend it forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that could be good for a v0.1 but isn't sufficient to be our target.
In my presentation on Monday, I mentioned potentially retrieving this registry from a remote endpoint, which would address the “outdated” concern. Antoine pointed out that node currently does not itself make any http requests, so this would be quite unusual. So I was thinking perhaps we could ask package managers for a very slight augmentation to their existing search functionality to support searching for loaders (ex npm search typescript --searchopts …
), which avoids reinventing the wheel and leverages a familiar experience; also, these seems an appropriate place for that functionality. I messaged @arcanis on Slack to check the temperature of the room, but haven't heard back yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having Node make an HTTP call will be a very hard sell, especially for this use case. It was proposed to have a command like node --check-version
to see if Node was running the latest version and that got shot down because of security concerns.
You could perhaps include in the prompt some text like “Run npx setup-typescript
“ and we can publish a setup-typescript
command to npm, that starts a wizard and prompts people through it, etc. I would still include a link to the docs, though, so that the only choice isn’t to just run unknown code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I heard back from Maël that the search we're looking for is actually already possible. I checked earlier tonight, and it seems this could do:
$ npm search --json module-translator typescript
We would need a specific tag for whatever we call this packages. module-translator
appears to be unused so far: https://www.npmjs.com/search?q=keywords:module-translator
test/es-module/test-esm-non-js.mjs
Outdated
await fs.mkdir(nodeModules, { recursive: true }); | ||
await fs.cp(fixtures.fileURL('external-modules', 'web-loader'), path.join(nodeModules, 'web-loader'), { recursive: true }); | ||
await fs.writeFile(path.join(cwd, 'main.ts'), 'const foo: number = 1; console.log(foo);\n'); | ||
await fs.writeFile(path.join(cwd, '.env'), 'NODE_OPTIONS=--import=web-loader\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this needs to be created dynamically rather than living somewhere inside the fixtures
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not actually 🤔 Something about flaky testing
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
doc/api/external_formats.md
Outdated
|
||
## Introduction | ||
|
||
Node natively understands a handful of formats (see the table in [Modules: Load hook][Load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs always use “Node.js”.
Node natively understands a handful of formats (see the table in [Modules: Load hook][Load | |
Node.js natively understands a handful of formats (see the table in [Modules: Load hook][Load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that correct? Node.js
refers to the project, and node
refers to the binary. The binary is what has the understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look across the docs, “Node.js” refers to the software. When we refer to the binary we say node
, and write it with backticks; but that’s only in the context of running a command, not generally referring to the binary as a stand-in for the software as a whole.
For example, see all the references to Node.js in https://nodejs.org/api/esm.html#enabling and following sections.
Here’s the reference: https://github.com/nodejs/node/tree/0cec82277c6fe3e9fbb3cf07324b9091d7f049db/doc#:~:text=Use%20Node.js%20and%20not%20Node%2C%20NodeJS%2C%20or%20similar%20variants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the heck am I remembering then 🤔
doc/api/external_formats.md
Outdated
customization hook]). Non-native / external formats require a translator (under the hood, this is a | ||
custom loader). When attempting to run a module with an external format (ex `node main.ts`), node | ||
will try to detect what it is. At current, when node is able to identify the format, it prints a | ||
message to stdout with instructions (that lead here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customization hook]). Non-native / external formats require a translator (under the hood, this is a | |
custom loader). When attempting to run a module with an external format (ex `node main.ts`), node | |
will try to detect what it is. At current, when node is able to identify the format, it prints a | |
message to stdout with instructions (that lead here). | |
customization hook]). A non-native or external format requires installing a dependency | |
to enable support for the additional format. | |
When attempting to run a module with an external format, such as `node main.ts`, Node.js | |
will look at the file extension to attempt to identify the file type. If the file type is | |
recognized, Node.js will print a message with instructions (that lead here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Node.js
here is also not correct—I think it should be node
. I can't remember the contributor guide that details the rules (I think @aduh95 quoted it to me before?).
doc/api/external_formats.md
Outdated
will try to detect what it is. At current, when node is able to identify the format, it prints a | ||
message to stdout with instructions (that lead here). | ||
|
||
## Setting up a module translator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t like the term “module translator.” It’s jargon without a well-known definition.
## Setting up a module translator | |
## Adding support for module formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need something unique and recognisable for the npm registry tag
doc/api/external_formats.md
Outdated
"css": "./css.mjs", | ||
"register": "registration.mjs", | ||
"typescript": "./typescript.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"css": "./css.mjs", | |
"register": "registration.mjs", | |
"typescript": "./typescript.mjs" | |
"register": "registration.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub mobile posted my earlier comment wrong—as a PR comment instead of an inline on these lines. See below.
doc/api/external_formats.md
Outdated
The filenames (the values in `"exports"`) can be whatever you want (see [package.json entry | ||
points][Package entry points]). | ||
|
||
`css.mjs` and `typescript.mjs` from the above example are [custom loaders][Module customization hooks]. They will need a [`resolve`][Resolve customization hook] hook that sets `format` to the | ||
format it handles (ex `'css'` and `'typescript'` respectively), a [`load`][Load customization hook] hook | ||
that translates the source of the external format to something node understands, and optionally an | ||
[`initialize`][Initialize customization hook] hook. | ||
|
||
`registration.mjs` registers your loader(s) with node (see [module.register][Module register]). It would look something like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filenames (the values in `"exports"`) can be whatever you want (see [package.json entry | |
points][Package entry points]). | |
`css.mjs` and `typescript.mjs` from the above example are [custom loaders][Module customization hooks]. They will need a [`resolve`][Resolve customization hook] hook that sets `format` to the | |
format it handles (ex `'css'` and `'typescript'` respectively), a [`load`][Load customization hook] hook | |
that translates the source of the external format to something node understands, and optionally an | |
[`initialize`][Initialize customization hook] hook. | |
`registration.mjs` registers your loader(s) with node (see [module.register][Module register]). It would look something like: | |
The filename (the value of `"register"`) can be whatever you want (see [package.json entry | |
points][Package entry points]). The registration file, `registration.mjs` in this example, | |
configures Node.js to load files that export module customization hooks as needed | |
to support the new module formats. For example: |
doc/api/external_formats.md
Outdated
register('example-translator/css'); | ||
register('example-translator/typescript'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register('example-translator/css'); | |
register('example-translator/typescript'); | |
register('./hooks/css.mjs', import.meta.url); | |
register('./hooks/typescript.mjs', import.meta.url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole package will likely be solely the hook(s), so I think this is not a realistic example 🤔
Also, why do it with a relative path + import.meta.url
instead of exports? Exports achieves the same result but far simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the whole package likely be solely the hooks? We’ve already heard from the ts-node
team and others that they need code on the main thread as well as the hooks thread. If that’s required for TypeScript, it’s likely to be the case for many other external formats as well. We should establish a best practice that’s flexible enough to support all libraries that need to fit into our template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point.
Is there a good reason though to use the relative + import.meta.url syntax here though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's not very verbose, and it's better than telling people to add new exports. Many people don't use exports
. And some authors will want to add this to a larger package like ts-node
rather than create a package just for this, and they might have their own patterns already for exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think there are pros and cons to each. Unless you feel strongly, I prefer mine for the examples in the docs because it's easier to grok, and grokability is critical for docs examples. Also, the import.meta.url
bit we already agree is a gotcha (but a currently necessary evil).
An author of ts-node will know the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs currently always use register
with import.meta.url
I believe, because it's often a mistake to do otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's not though, and it would merely add noise to the example. Exposing typescript
via exports
could have additional utility too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at https://nodejs.org/api/module.html#customization-hooks, all of the examples using register
include import.meta.url
. Someone can always provide exports and use import.meta.url
. We should include import.meta.url
in the example, because it only helps and it’s the best practice we’re trying to encourage for when people use register
. I think most authors are much more familiar with referencing a file relative to the current file than they are referencing a file based on an exports
map entry.
/** | ||
* @typedef {string} FileExtension The extension of a file, including the leading dot. | ||
* @typedef {string} FormatName A tag-friendly name for a module format. This is used to search for | ||
* a module translator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* a module translator. | |
* a package that provides module customization hooks to support this format. |
doc/api/external_formats.md
Outdated
customization hook]). Non-native / external formats require a translator (under the hood, this is a | ||
custom loader). When attempting to run a module with an external format (ex `node main.ts`), node | ||
will try to detect what it is. At current, when node is able to identify the format, it prints a | ||
message to stdout with instructions (that lead here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Node.js
here is also not correct—I think it should be node
. I can't remember the contributor guide that details the rules (I think @aduh95 quoted it to me before?).
doc/api/external_formats.md
Outdated
will try to detect what it is. At current, when node is able to identify the format, it prints a | ||
message to stdout with instructions (that lead here). | ||
|
||
## Setting up a module translator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need something unique and recognisable for the npm registry tag
doc/api/external_formats.md
Outdated
1. Once installed, in order to get node to automatically use it, you will need to create a `.env` | ||
file containing a [`--import`][CLI import] for your chosen module translator's package (ex `NODE_OPTIONS="--import=example-translator/register"`). See [CLI: | ||
env file][CLI env file]. | ||
1. When you subsequently run node, include the env file flag (ex `node --env-file=.env main.ts`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this instructs the user to manually complete a process in the same way it will be automated in future (so no migration will be needed when node starts automatically loading ./.env
). Doing it this way, eventually the user can stop doing something instead of change what they need to do.
doc/api/external_formats.md
Outdated
## Publishing a module translator | ||
|
||
A few specific things are necessary when publishing a module translator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woof, that heading is super verbose.
Also Node.js
vs node
.
doc/api/external_formats.md
Outdated
|
||
### Package registration | ||
|
||
Your package registration needs the following tags (aka keywords): `module translator`, `node`, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodejs-external-format-support
😬 🙅♂️
doc/api/external_formats.md
Outdated
|
||
### Package registration | ||
|
||
Your package registration needs the following tags (aka keywords): `module translator`, `node`, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And tell them where to put these; in package.json keywords?
I don't know how these get assigned to the npm registration—be they extracted from package.json or explicitly configured with npm. I think that's outside the scope of this doc (and should be handed by npm's docs).
doc/api/external_formats.md
Outdated
"css": "./css.mjs", | ||
"register": "registration.mjs", | ||
"typescript": "./typescript.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduh95 I think this is what we talked about, but now I'm wondering if the exports should be individual registrations (ex css.mjs
contains the module.register()
), and then individually specified (--import=example-translator/css --import=example-translator/typescript
).
doc/api/external_formats.md
Outdated
"css": "./css.mjs", | ||
"register": "registration.mjs", | ||
"typescript": "./typescript.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub mobile posted my earlier comment wrong—as a PR comment instead of an inline on these lines. See below.
doc/api/external_formats.md
Outdated
|
||
### Package setup | ||
|
||
Your package exports must contain a `"register"` key and should also contain keys for the external format(s) it handles, like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should. I'm intending to set a best-practice. But actually, I'm thinking maybe we don't want the catch-all but more granular registrations (see below—I can't link to the comment because it's pending with the rest).
doc/api/external_formats.md
Outdated
register('example-translator/css'); | ||
register('example-translator/typescript'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole package will likely be solely the hook(s), so I think this is not a realistic example 🤔
Also, why do it with a relative path + import.meta.url
instead of exports? Exports achieves the same result but far simpler.
@GeoffreyBooth sorry, Github is having a stroke and posting comments in new and separate threads. |
There’s not really anything specific to external formats here other than the fact that Node is prompting the user based on an unrecognized file type. We could make this more general, to cover any package that provides module customization hooks (or even customization hooks broadly, for any subsystem, like once we have filesystem hooks and REPL hooks and so on). In the future there could be other ways to prompt this flow, like I would pick a tag that’s specific to us and flexible to support future use cases such as this. Maybe |
Oow, good idea! I originally called them "extensions" (since they both extend node's functionality and here inspect a file extension). But "plugin" would also be fine with me (it might be a little more common?). In that case, should this be moved into the same doc as module customisation? |
If this becomes a general customization method, shouldn't it be it's own top level thing? 'Customizations' or something? |
Perhaps. Should we do that now or altogether later? |
3c374d8
to
51f0b2e
Compare
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Better to create a new top-level section once rather than create something with one name now and then rename it later. Calling it “Customizations” would align with nodejs/loaders#95, but I don’t know if we want to be that generic since I think it might be more appropriate for sections like “filesystem customization hooks” and “REPL customization hooks” and so on to live in the |
Hrm. What if we had a TL "Customizations" section that explains the common stuff, keep the fs et al docs within their subsystems doc, and list links to those specific customisation features from the Customizations doc? Customizations
A APIs |
Yes, that’s exactly what I was thinking. And I feel how painful this will be for you to use so much U.S. English spelling for this new page 😄 Besides links to other sections, this new page could include the overall “how to write a plugin” guide that you have in this PR, since that part should apply to all the subsystems. And we would need to write it to be generic, not focused on the “support additional file types” use case. So it couldn’t have the “add |
Hey @JakobJingleheimer, what's the reason for abandoning this PR? I'd like to take a look |
Do I understand correctly? The If yes: Is that even needed? Couldn’t Node.js simply use the
|
No, the intent wasn’t to change the behavior of More like, if this is opted into somehow (config setting?) automatically run |
Got it, thanks for explaining! It’s great to see Node.js considering how using TypeScript can be improved (which is an interesting challenge – given tsconfig etc.). |
This unfortunately fell victim to death by committee. |
But only this particular way of providing this functionality, right? As the “plugin API” evolves, the same functionality can be provided differently(?) |
No, the concept of a "plugin API" is effectively dead. |
No proposal is ever really dead. This can get revived at any time if someone wants to pick it up. There’s another proposal about creating a config file for Node. If that progresses, it could pair with this nicely where instead of magic names like |
Phase 1