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 #2221

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

guybedford
Copy link
Member

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?

@guybedford guybedford merged commit 6e8cbd5 into master Jul 30, 2020
@github-actions
Copy link

core

File by file impact

File Transform Diff master auto-import-race Event
dist/s.js none -175 21,829 21,654 changed
gzip -131 6,211 6,080
brotli -87 5,342 5,255
dist/s.min.js none +51 6,562 6,613 changed
gzip +57 2,601 2,658
brotli +42 2,339 2,381
dist/system.js none -175 32,275 32,100 changed
gzip -126 8,711 8,585
brotli -94 7,540 7,446
dist/system.min.js none +51 10,587 10,638 changed
gzip +58 4,044 4,102
brotli +50 3,617 3,667
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.

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.

None yet

2 participants