-
-
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
Fix auto import dynamic imports during loading #2245
Conversation
File size impactbrowser (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. |
@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. |
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.
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.
@guybedford I'm on it |
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:
It may be necessary to make a related change to the fetch loader in due course to support auto imports as well.