-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add ambient-loader proposal #63
Changes from 5 commits
ed0fab9
96ecc5e
706ea20
f403fd6
630173d
f4c816b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
# Ambient Loaders Design | ||
|
||
## Problem | ||
|
||
Loaders let side logic be applied when resolving import statements. However, this only applies to the main application: loaders themselves aren’t currently affected by any other loaders. As a result, 3rd-party loaders cannot be injected if accessing them requires going through another loader. | ||
|
||
For instance, imagining a loader that would allow modules to be loaded from a `node_modules.zip` file (instead of the typical folder), the following wouldn’t work: | ||
|
||
``` | ||
node --loader zip --loader ts-node ./my-tool.mts | ||
``` | ||
|
||
Indeed, importing `ts-node` would require the `zip` loader to contribute to the resolution (so that the final path loaded is `$PROJECT/node_modules.zip/ts-node` instead of `$PROJECT/node_modules/ts-node`, which doesn't exist), but it’s not what happens today: both `zip` and `ts-node` are resolved by the default Node resolver, preventing `ts-node` from resolving. | ||
|
||
The reason for that is an attempt to keep loaders as isolated as possible, to allow followup improvements like reusing them across workers or scaling them up. If all loaders were influenced by prior loaders, they’d effectively coalesce into a single one, which would prevent such work. Still, the problem remains. | ||
|
||
## Proposal | ||
|
||
If loaders cannot generally influence each other, we could have a subset of them do. Indeed, not all loaders affect the resolution so much that they are a requirement to followup loaders. We could have two levels of loaders: | ||
|
||
- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially and would affect the resolution of all subsequent ambient and regular loaders. | ||
|
||
- Regular loaders would be defined via the `--loader <module>` flag. They would be loaded in parallel (at least conceptually). Because they’d only be loaded after the ambient loaders have finished evaluating, their resolution would be affected by ambient loaders. | ||
|
||
## Simple Example | ||
|
||
Let's imagine we have the following loaders: | ||
|
||
- ts, which adds support for TS files | ||
- my-custom-loader, which is written in TypeScript | ||
|
||
Without the TS loader, we wouldn't be able to even load the following custom loader. But by marking it as an ambiant loader, Node will make sure it'll be taken into account when resolving (and loading) my-custom-loader. | ||
|
||
|
||
The command line would end up like this: | ||
|
||
``` | ||
node --ambient-loader ts | ||
--loader my-custom-loader | ||
./path/to/script.mjs | ||
``` | ||
arcanis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## More Contrived Example | ||
|
||
Let’s imagine we have the following loaders: | ||
|
||
- zip, which adds zip file support to the `fs` module | ||
- pnp, which adds support for the Plug’n’Play resolution ([more details](https://yarnpkg.com/features/pnp)) | ||
- yaml, which adds yaml file support to `import` | ||
- coffeescript, which adds coffeescript file support to `import` | ||
|
||
The first two are critical to a successful resolution: without `zip`, PnP wouldn't be able to load files from the zip archives. Without `pnp`, Node would look for the `yaml` and `coffeescript` loaders into a `node_modules` folder, which would fail. On the other hand, `yaml` and `coffeescript` don't depend on each other. | ||
|
||
The command line would end up like this (in practice `--ambient-loader` would probably be passed via `NODE_OPTIONS` rather than directly on the command line): | ||
|
||
``` | ||
node --ambient-loader zip | ||
--ambient-loader pnp | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there is some debate about the correct ordering of these, based on Slack discussion: https://node-js.slack.com/archives/CEVD3CX33/p1647532211808939 The competing factors are a) which ordering is more intuitive, and b) if ordering should be done such that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, thinking about this some more, maybe the ordering in this particular example doesn't matter. |
||
--loader yaml | ||
--loader coffeescript | ||
./path/to/script.mjs | ||
``` | ||
|
||
When resolving `coffeescript`, we’d go into the `pnp` loader (but not `coffeescript`, which isn’t an ambient loader). The `pnp` loader itself would have been loaded after the `zip` loader ran, causing its `fs` import to be resolved to a custom module adding zip support to `fs` rather than the usual builtin module. | ||
|
||
arcanis marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
Are we 100% certain that we cannot let regular loaders be influenced by prior loaders? I'm afraid that if ambient loaders seem more powerful than regular loaders, we will just end up with everyone always using
--ambient-loader
and forget about the other flag.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 we said no because it would require reentrancy, and our preliminary thoughts on this Ambient Loader concept was that it's the nuclear option so it would be chosen very infrequently (too easy to foot-gun, so only worth it when you really need it and really know what you're doing).
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'd be curious to hear about reentrancy hazards that will crop up in the types of loaders that can be non-ambient. E.g. a use-case where you don't need
--ambient-loader
and yet are doing something tricky enough that you're likely to footgun. A simple source->source compiler such as a coffeescript loader wouldn't have any of these risks, I don't think.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.
@arcanis If you feel comfortable explaining it, do you mind adding a section to the document about reentrancy and its hazards and what this means for loaders/ambient loaders? Like perhaps how those issues can be addressed (or not) via ambient loaders, etc. Or what if we only allowed just one ambient loader?
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.
As I mentioned in this thread, I'm not sure myself I see the problems with reentrancy (nor I see it avoidable for resolution in general, or particularly affected by ambient loaders), so I'd prefer if someone else could make a writeup.
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.
Who is concerned about these hypothetical reentrancy concerns? From an outside perspective, it appears as if perhaps it was hypothesized but no one could think of any examples.
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.
@bmeck was the one who knew the most about them.
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'm also concerned about reentrancy, but for me that doesn't block adding the proposal itself, just a concern about a later stage.
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.
btw, I'm not blocking this proposal in any way. I do not have the bandwidth to participate in meetings, so I try to at least follow what's happening in this repo. I trust that you all are doing the right thing in the end 👍🏻