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

Auto import and core refactoring #2216

Merged
merged 2 commits into from Jul 23, 2020
Merged

Auto import and core refactoring #2216

merged 2 commits into from Jul 23, 2020

Conversation

guybedford
Copy link
Member

This implements the auto import approach directly in core, as created by @tmsns in #2210, extending it to any number of scripts.

In addition this includes core refactoring to make processing phases more clear for import map and script processing.

@github-actions
Copy link

core

File by file impact

File Transform Diff master auto-import Event
dist/s.js none +1,614 19,518 21,132 changed
gzip +583 5,490 6,073
brotli +465 4,751 5,216
dist/s.min.js none +189 6,085 6,274 changed
gzip +104 2,408 2,512
brotli +77 2,171 2,248
dist/system-node.cjs none -1,598 202,359 200,761 changed
gzip -529 51,824 51,295
brotli -338 43,746 43,408
dist/system.js none +1,614 29,964 31,578 changed
gzip +559 8,016 8,575
brotli +456 6,951 7,407
dist/system.min.js none +189 10,110 10,299 changed
gzip +93 3,860 3,953
brotli +66 3,462 3,528
extras

File by file impact

Pull request have no impact on extras files.

Generated by github pull request filesize impact

@guybedford guybedford mentioned this pull request Jul 23, 2020
@guybedford guybedford merged commit 26f91ca into master Jul 23, 2020
@guybedford guybedford deleted the auto-import branch July 23, 2020 19:41
@guybedford
Copy link
Member Author

Released in 6.4.0.

@tmsns
Copy link

tmsns commented Jul 29, 2020

Thanks for implementing this refactor! 😊

I was able to do some more indepth tests and the basic functionality works. However, we have some race conditions. I'll point it out, in the code.

systemJSPrototype.register = function (deps, declare) {
if (hasDocument && document.readyState === 'loading' && typeof deps !== 'string') {
var scripts = document.getElementsByTagName('script');
var lastScript = scripts[scripts.length - 1];
Copy link

Choose a reason for hiding this comment

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

Unfortunately, when using the condition document.readystate === 'loading', it is not guaranteed that the last script of the page is indeed the one who called register(). More specifically, this can happen when the first auto-imported script is calling a dynamic import (System.import()) while there are still some script tags being parsed in the page (either statically or dynamically)

I'll try to come up with a way to differentiate these dynamic imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh that makes sense. We should be able to specifically opt-out of SystemJS own-injected scripts from this mechanism somehow.

Copy link

@tmsns tmsns Jul 29, 2020

Choose a reason for hiding this comment

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

Actually, this is very similar to the problem discussed below 🙈 , except that:

  • the user is dynamically injecting a script using a regular dynamic load (System.import) during load, which will cause a System.register later on ofc
  • the last script was not related to the System script, but another script at the end of the page

In other words, it doesn't seem such an edge case. 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh weird, maybe that code path isn't working for some reason? I didn't fully test it yet - but yes that logic should be doing the avoidance here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure your race conditions aren't just the expected ordering ones?

Copy link

Choose a reason for hiding this comment

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

Well, in my setup I have only one auto-imported script. In that script, there is a dynamic import to load the main chunk of the page. When System.register of the chunk is being called, I can see it takes the src of an unrelated script further down the page. So, in that sense, I don't expect any other ones. 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok then it sounds like this top-out path is not working as it is supposed to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect the issue is that lastScript is being checked in that dynamic import script, and not matching its own script as it is dynamic.

A timing-based fix with the registration system should be possible here... I will aim to take a look.

Copy link

Choose a reason for hiding this comment

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

Yup, indeed. I don't think there is a full proof way in IE to distinguish System.register's being called in the dynamic-import way vs the auto import way, as document.currentScript cannot exactly be faked.

I see a couple of workarounds though, but they are never ideal:

  • Disable auto-import once a first dynamic import is being called.
  • Add a second condition next to document.readyState === 'loading' to check a specific attribute of the lastScript.
  • Have dynamic imports only actually register (and not import) during the load phase of the element. This is a big change though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use the existing mechanism which is based on the sync timing of the register callback itself to register the script to the right URL. Integrating this with this code path will involve some timing tricks though and likely requires moving all this logic into an async step with checks.

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