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
esm: remove CLI flag limitation to programmatic registration #48439
esm: remove CLI flag limitation to programmatic registration #48439
Changes from 2 commits
28055ff
d4a5dd8
378c5a5
d2ad85b
02c6090
cb72d94
77e91dc
582f50a
ec5d529
e6e8cac
3819d08
9ff1de7
6fa7dc7
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.
esmLoader
getter directly callscreateModuleLoader
if not exist, which makes it really hard to keep track of the execution flow. Would you mind adding some comments about the assumption of the state ofesmLoader
attribute in 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.
On Slack we were discussing creating a function
getModuleLoader
which would return the current active module loader instance, whether it was an instance of the customized or the default one.Also in general this PR isn’t ready for review yet, I think it’s just where @JakobJingleheimer finished at the end of the night. It doesn’t work yet, and we’re not sure which of the two approaches described in the top post will work or be more readable.
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.
Yeah, it's a draft. Not sure why GitHub tagged people for review.
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.
It's the bot who does the tagging, not GitHub. The webhook is executed when you open a PR, regardless of its status. You probably already know that, but you don't have to open a PR, you can also push commits to your forks and open a PR later if you don't want comments for now.
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, it doesn't have to be configured that way (at work, we have the same kind of thing, and it only runs when the PR is not a draft—eg opened as non-draft or switches from draft to non-draft).
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 didn’t realize there was a conversation was going on Slack. I mostly left these comments because I’m trying to familiarize myself with the loader implementation and I believe asking/commenting is the best way of learning.
Sorry for any disturbance caused by my comments.
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 just meant this isn't ready for cleanup notes yet. We need to figure out which approach to take and get it working. If you have suggestions for that (see initial post) please feel free 😀
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.
Nit:
createModuleLoader
makes the final decision of whether default module loader is created or customized module loader is created. But this comment and this function is called when we know CustomizedModuleLoader is going to be created. I think we should decouple the condition of conditionally creating customized or default module loader creation to their own functions and directly create them. This would potentially improve the readability and maintainability of loaders, since if in the future the behavior ofcreateModuleLoader
changes, we also need to update this line of comment.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.
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 import attributes may be modified by a user loader, so using
kEmptyObject
wouldn't fly here. Also that code was there before this PR. (maybe let's not make unrelated whitespace changes?)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 changed multiple args to the constructor, so it wasn't just fixing the whitespace mangling. If you'd like the mangling fixed separately, I can re-mangle the 2 lines and fix in a follow-up (but the purpose of this PR is also clean-up that was deferred from the previous PR).
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.
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 this does not matter here: we provide and destructure known keys, so it would never reach the prototype.
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.
that would mean it doesn't matter now, but if anyone ever read a new key in the future without adding that key to every single callsite, it'd be a vulnerability.
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.
That's valid but a larger issue, that I think is out of scope 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.
Is there any advantage to not coding defensively here, especially in the module system?
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 @aduh95 recently said it's a de-op
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 say it can be a de-op only if you sometimes pass objects with null prototype and sometimes don't.
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.
Let’s please not haggle over cleanup stuff yet. We need to get this PR working first.
Also isn’t there a lint rule for
__proto__: null
? So this will get fixed before the PR lands, in order to get the lint check to 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.
The linter rule only enforces which style is used for null objects - it doesn't enforce that null objects are used.
Totally fair tho to not haggle over cleanup stuff yet; there's no rush to address this.