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

Fix auto import dynamic imports during loading #2245

Merged
merged 3 commits into from
Sep 12, 2020

Conversation

guybedford
Copy link
Member

This fixes the issue posted at #2243 (comment) that the code to ignore new dynamic imports when checking the auto import list during the loading phase was happening too late - after the new dynamic import with the auto import cache had already been checked.

This brings back the timeout method previously removed in #2225 with a slightly simpler approach.

It appears the script list is not a valid replacement for currentScript because of dynamic injection cases like this. In particular we inject the script into the head, so that document writes in the body will always be later resulting in dynamic imports being incorrectly assigned over the auto imports of body scripts that are currently at the document write position.

While it does on the surface seem a problem to have lost this invariant, as far as I can tell I think we are still on top of a safe foundation for this feature, based on the remaining (and now revised) assumptions of its implementation:

  1. Any script which does a System.register during load that was not loaded by SystemJS is considered an auto import
  2. Those scripts get their URL detected by being the last "script" element on the page, this means it holds true for all static scripts and document.write scripts.
  3. This PR fixes up the detection of System.register injected scripts made by SystemJS itself to ensure they are comprehensively never treated as auto imports.
  4. Any other type of dynamic injection is fine and does not interfere with the above process (and this is why we don't need to have a true currentScript polyfill and can get away with the lack of one in IE11) - because if they don't call System.register, they are effectively invisible to this process. And if they do call System.register, that is breaking the invariant of the assumptions of this implementation that all System.register scripts are either static auto imports or loaded via SystemJS itself.

It may be necessary to make a related change to the fetch loader in due course to support auto imports as well.

@github-actions
Copy link

github-actions bot commented Sep 11, 2020

File size impact

browser (0)

No impact on files in browser group.

node (0)

No impact on files in node group.

extras (0)

No impact on files in extras group.

Generated by file size impact during size-impact#250606010

@guybedford
Copy link
Member Author

@dmail the size impact update here seems to be missing the SystemJS and S.js builds. If you have any ideas why this has changed please let me know.

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.

I haven't kept fully up-to-date on all the auto-import changes, and I don't know all the implications of this change. But this looks good to me.

src/features/script-load.js Outdated Show resolved Hide resolved
@guybedford guybedford merged commit 1f36179 into master Sep 12, 2020
@guybedford guybedford deleted the fix-auto-import-dynamic branch September 12, 2020 00:11
@dmail
Copy link
Contributor

dmail commented Sep 13, 2020

@dmail the size impact update here seems to be missing the SystemJS and S.js builds. If you have any ideas why this has changed please let me know.

@guybedford I'm on it

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