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

Add ambient-loader proposal #63

Merged
merged 6 commits into from
Mar 27, 2022
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions doc/design/proposal-ambient-loaders.md
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.
Copy link
Member

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.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Feb 15, 2022

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 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).

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

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.

@bmeck was the one who knew the most about them.

Copy link
Member

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.

Copy link
Member

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 👍🏻


## 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 argv takes precedence over NODE_OPTIONS

Copy link
Contributor

Choose a reason for hiding this comment

The 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