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 2 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
45 changes: 45 additions & 0 deletions doc/design/proposal-ambient-loaders.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# 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, the following doesn’t work:

```
node --loader pnp --loader ts-node ./my-tool.mts
arcanis marked this conversation as resolved.
Show resolved Hide resolved
```

Indeed, importing `ts-node` would require the `pnp` loader to contribute to the resolution, but it’s not what happens today: both `pnp` 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 followup loaders.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I registered two ambient loaders, would the first one affect the resolution of the second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

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.

Oh, wait, what. I did not get that from this doc. I understood ambient loaders to work in tandem like how regular loaders do (one regular loader does not affect the other), just specifically when loading a regular loader.

Suggested change
- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup 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 loaders.

or possibly

Suggested change
- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup 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.

Making ambient loaders affect each other sounds like a pandora's box.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with your second wording

Making ambient loaders affect each other sounds like a pandora's box.

Why that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with your second wording

Thanks!

Making ambient loaders affect each other sounds like a pandora's box.
Why that?

Wouldn't that open the door for reentry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm still not sure I see a problem with reentrance, we may have different behaviours in mind 😃

But in this case it shouldn't be reentrant anyway: it's just using the result of the previous loader to load the next one, so all execution occurs in the same order and we don't go twice in the same loader during the same resolution pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when one ambient loader depends on another in a circular fashion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example?

The closest I can think of would be if, say, we had /foo.zip/bar.tgz/qux.zip/index.js, but isn't this pattern true as well for regular module imports?


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

## 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
arcanis marked this conversation as resolved.
Show resolved Hide resolved
- 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