Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add ambient-loader proposal #63
Changes from 2 commits
ed0fab9
96ecc5e
706ea20
f403fd6
630173d
f4c816b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 👍🏻
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 I registered two ambient loaders, would the first one affect the resolution of the second?
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.
Yes.
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.
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.
or possibly
Making ambient loaders affect each other sounds like a pandora's box.
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.
Updated with your second wording
Why that?
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.
Thanks!
Wouldn't that open the door for reentry?
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.
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.
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 about when one ambient loader depends on another in a circular fashion?
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.
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?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 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 overNODE_OPTIONS
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.
Actually, thinking about this some more, maybe the ordering in this particular example doesn't matter.