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