-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixes for named-register extra. #2352
Conversation
File size impactMerging issue-2349 into main will impact 5 files in 3 groups. browser (2/2)
node (1/1)
extras (2/8)
|
@guybedford do you have time to look at this one? If not, should I go ahead and merge? |
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.
This looks great! Perhaps we can actually just make this a minor release, I would really prefer not to do a new major. If you think there is a very high risk of breakage, is there perhaps a way we can keep the old behaviour and have a flag for the new behaviour for the next major?
And sorry, this dropped off my notifications as SystemJS notifications bundle together for me here. |
I think we could call this a bug fix rather than a breaking change? This was the part that I was thinking might be a breaking change for some, but only because they're accidentally relying on buggy behavior. |
Ok yes, let's do it as a bug fix then. |
This resolves #2349. It fixes two bugs:
Demonstration of the bugs:
The changes I had to make to solve these bugs were more invasive than I was hoping for. It might be safest to release this is a new major version, as people are probably accidentally relying on the named-register extra to sometimes return the first named register and other times the last named register.
I'm open to all feedback on this one, as it was quite difficult and I tried several approaches before finding one that worked in all cases. The first bug is solved by creating a new
namedRegisterAliases
property on systemjs instances, to ensure that only one module is instantiated. The second bug is solved by switching fromPromise.resolve
tosetTimeout
, since theload
event for<script>
events seems to run after thePromise.resolve()
executes.Adding the url to the getRegister API was something I wanted to avoid, but ultimately was the least invasive approach I found that worked.
@guybedford I would appreciate your review on this one, if you're able to.