-
-
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
Update auto import to handle dynamic import races #2223
Conversation
coreFile by file impact
extrasFile by file impactPull request have no impact on |
I did some first tests, and unfortunately the situation is worse now: the import of the static entry is being delayed. Tomorrow, I'll test with the dynamic import, but it doesn't look good. |
Thanks for testing this out. Please let me know how it works on the dynamic import cases when you can. Will see if there are any ways to shorten the microtask delay. |
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.
Clever approach
Co-authored-by: Joel Denning <joeldenning@gmail.com>
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.
Looks good to me!
Released in 6.4.1. @tmsns unfortunately blocking rendering is not a goal as SystemJS follows module semantics. It might be an option to try and reduce the delay though by eg using Please do let me know if you can check the races here too. |
So, I just finished testing and all seems to be working well 😊 Both for the races in the dynamic imports and the blocking aspect of it. I'm gonna try to promote this next week for more extended testing. 👍 Thanks for the fix! |
@tmsns thanks for confirming, great to hear that! |
I understand what you mean., but I'd like to add a sidenote here. When using bundlers like Rollup, one can choose the output format, eg |
If you have a script that does not use import or export syntax, then use a dynamic |
True, that’s the other possibility. :-) I wonder if Rollup would be open for this kind of change... |
It's very straightforward as a plugin with the |
Thanks Guy! Food for a next simplification in the future.. :-) |
Sorry to come back to this PR, but we have a new race issue. More specifically, when the page loads too fast, the auto-imported script is being removed from the candidates list (as the list resets: https://github.com/systemjs/systemjs/pull/2223/files#diff-b8033cc8030ffe48fef95bb6295a0c21R40). This retriggers the download and execution of the script. Even worse, if the file has been bundled with SystemJS (as only 1 download was one of the initial advantages of the plugin), SystemJS gets reinitialized as well, breaking dynamic imports. I was able to reproduce the issues here: @guybedford let me know if you want me to create an actual issue or if you need help in any way (coming up with a solution, or testing) 😊 |
Reposting #2221 which was merged by mistake.
As discussed in #2216 (comment).
The approach taken is to wait one tick before completing the auto import. In that time the script loader path is able to then opt-out any auto imports if they have been script loaded.
@tmsns I wasn't able to replicate the race condition unfortunately, but it should now handle it correctly I believe. Any chance you could test this PR out there and confirm?