-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add basic import maps support #8075
Conversation
This imports much of the specification from https://wicg.github.io/import-maps/. The main deltas are: * Does not include support for external import maps. This allows a good deal of simplification, such as by changing "acquiring import maps" + "pending import map script" + "wait for import maps" to a single "import maps allowed" boolean, or integrating "register an import map" into "execute the script element". If a src="" attribute is present on an importmap script element, then an error event is fired. * Does not include any support for non-Window import maps. The spec draft included stub support for those, with other globals always having an empty import map. This instead ties import maps to Windows directly. If we revisit that in the future we can refactor, but keeping always-empty import maps around on the other globals was clumsy. * Adds introductory text and examples. * Adds conformance requirements, both on the script element and on the import map JSON. * Fixes a few minor specification issues: WICG/import-maps#267, WICG/import-maps#267, WICG/import-maps#268, WICG/import-maps#270, WICG/import-maps#276, WICG/import-maps#277.
I think webkit has feature bug already? https://bugs.webkit.org/show_bug.cgi?id=220823 |
@domenic |
I agree the preparation-time document check will never fail for import maps. This is a general programming pattern where you put shared logic inside a shared function (in this case "execute the script element"), even if for some inputs to that function, the logic is not necessary. I don't think it's a good idea to restructure the specification so that execution of the import map happens during the preparation phase, instead of the execution phase. Even if the results are observably the same, it makes the specification much harder to read and follow. |
Sorry for delay, LGTM. My comments are editorial except for #8075 (comment). |
Thanks for the review @hiroshige-g! I think I have addressed everything. Since there is no big rush, I will wait for co-editor review from @annevk when he gets back, before merging. In the meantime I can do a bit of work on tidying up the tests. |
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.
This looks really good. I did not verify all logic, but I did read through it all. I have a couple minor suggestions as a result.
source
Outdated
<p class="note">Import maps are currently disallowed once any module loading has started, or once | ||
a single import map is loaded. These restrictions might be lifted in future specification | ||
revisions.</p> |
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.
This setup seems like it would allow for races. Shouldn't we call this the moment we see a <script>
element that's not an import map or when we see a dynamic import()
? At least, I don't think module loading necessarily starts when a <script>
element is seen.
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 guess you are referring to the period between when you see a <script type=module>
in the parser, and the later period (by default, defer timing) where loading starts? I think that's fine, and still deterministic. (Unless you opt in to nondeterminism with async
.)
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.
It seems nicer to just build upon element insertion time.
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 think I disagree. It feels nicer to centralize the trigger point in the module loading algorithm, and it's nicer that scripts don't have any effect until they start fetching. E.g. it'd be weird if you inserted a script, then removed it before it started fetching, but it still blocked you from using import maps.
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.
But now if you have an async script and then insert an import map it may end up working and depending on the environment it might always work for you, but then break down for a couple of users sometimes.
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.
Yes, there are tradeoffs. But I think this direction is aligned with the general nature of async scripts, which if they depend on or change the environment, are likely to break nondeterministically.
How should we resolve this? To my knowledge all three implementations followed the spec, and it's tested, so we should probably proceed with merging and then open an issue to discuss changing, measuring the compat impact, etc.?
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 suppose that's fair. I don't think it's worth tracking this further unless others share my concern. Thanks!
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.
Not going to block on the async race thing, but I'm not a big fan.
Ignoring that this looks great and I'd be happy to see it properly standardized.
is it intended it got merged into |
Nope, sorting that out now over in #8072. |
This imports much of the specification from https://wicg.github.io/import-maps/. The main deltas are:
Does not include support for external import maps, since nobody has implemented these yet. This allows a good deal of simplification, such as by changing "acquiring import maps" + "pending import map script" + "wait for import maps" to a single "import maps allowed" boolean, or integrating "register an import map" into "execute the script element". If a src="" attribute is present on an importmap script element, then an error event is fired.
Does not include any support for non-Window import maps. The spec draft included stub support for those, with other globals always having an empty import map. This instead ties import maps to Windows directly. If we revisit that in the future we can refactor, but keeping always-empty import maps around on the other globals was clumsy.
Adds introductory text and examples.
Adds conformance requirements, both on the script element and on the import map JSON.
Fixes a few minor specification issues: "parse a URL-like import specifier", the term "import specifier" is a little bit confusing WICG/import-maps#267, Spec doesn't clearly define onerror must be called if import map is added after the 1st module load WICG/import-maps#268, Typo in "A import map is a struct with two items:" WICG/import-maps#270, preparation-time document check should be done first at #register-an-import-map WICG/import-maps#276, Throwing TypeError in resolve a module specifier? WICG/import-maps#277.
Review from @hiroshige-g and @allstarschh appreciated. The new flow in "prepare the script element" and "execute the script element" in particular needs careful review.
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/scripting.html ( diff )
/webappapis.html ( diff )