Skip to content
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

Merged
merged 2 commits into from
Jul 31, 2020
Merged

Conversation

guybedford
Copy link
Member

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?

@github-actions
Copy link

github-actions bot commented Jul 30, 2020

core

File by file impact

File Transform Diff master auto-import-race Event
dist/s.js none -178 21,829 21,651 changed
gzip -132 6,211 6,079
brotli -94 5,342 5,248
dist/s.min.js none +49 6,562 6,611 changed
gzip +54 2,601 2,655
brotli +38 2,339 2,377
dist/system.js none -178 32,275 32,097 changed
gzip -128 8,711 8,583
brotli -96 7,540 7,444
dist/system.min.js none +49 10,587 10,636 changed
gzip +57 4,044 4,101
brotli +49 3,617 3,666
extras

File by file impact

Pull request have no impact on extras files.

Generated by github pull request filesize impact

@tmsns
Copy link

tmsns commented Jul 30, 2020

I did some first tests, and unfortunately the situation is worse now: the import of the static entry is being delayed.
The "tick" you implemented using setTimeout is creating a macrotask (https://javascript.info/event-loop). This task is allowing the browser to render already, deferring the import of the first entry. This kinda defeats the purpose of using a regular script element in the first place. In IE11, it seems to be even worse as the task is being pushed after all script tags of the page. :-/

Tomorrow, I'll test with the dynamic import, but it doesn't look good.

@guybedford
Copy link
Member Author

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.

Copy link
Collaborator

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever approach

build-node.js Show resolved Hide resolved
src/features/script-load.js Show resolved Hide resolved
src/features/script-load.js Outdated Show resolved Hide resolved
Co-authored-by: Joel Denning <joeldenning@gmail.com>
Copy link
Collaborator

@joeldenning joeldenning left a 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!

@guybedford guybedford merged commit 23a3e00 into master Jul 31, 2020
@guybedford guybedford deleted the auto-import-race branch July 31, 2020 16:21
@guybedford
Copy link
Member Author

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 Promise.resolve().then or another tick mechanism with a shorter delay. Please let me know if you have any suggestions here, and apologies if this messes with your use case. The previous concept should still work as an extra though that you might be able to apply manually.

Please do let me know if you can check the races here too.

@tmsns
Copy link

tmsns commented Jul 31, 2020

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!

@guybedford
Copy link
Member Author

@tmsns thanks for confirming, great to hear that!

@tmsns
Copy link

tmsns commented Jul 31, 2020

@tmsns unfortunately blocking rendering is not a goal as SystemJS follows module semantics.

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 system or es. With System, this means that every file will have the System.register wrapper. When choosing es, the resulting file can be used both in a normal script tag and a module script. When using the normal one the script can be blocking, but features like dynamic import still work.
In that sense, it would have beeen nice that code wrapped with System.register would behave in the same way as es. 😊

@guybedford
Copy link
Member Author

If you have a script that does not use import or export syntax, then use a dynamic import transform into System.import only and don't output system at all.

@tmsns
Copy link

tmsns commented Jul 31, 2020

True, that’s the other possibility. :-)

I wonder if Rollup would be open for this kind of change...

@guybedford
Copy link
Member Author

It's very straightforward as a plugin with the renderDynamicImport hook - http://rollupjs.org/guide/en/#renderdynamicimport.

@tmsns
Copy link

tmsns commented Jul 31, 2020

Thanks Guy! Food for a next simplification in the future.. :-)

@tmsns
Copy link

tmsns commented Aug 5, 2020

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:
https://plnkr.co/edit/Pnpo79DZjpQL7xOg

@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) 😊

@guybedford
Copy link
Member Author

@tmsns thanks for the replication and case here, usually better to create an issue but it's no problem. I've put together #2225 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants