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

Add basic import maps support #8075

Merged
merged 7 commits into from
Oct 5, 2022
Merged

Add basic import maps support #8075

merged 7 commits into from
Oct 5, 2022

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 5, 2022

This imports much of the specification from https://wicg.github.io/import-maps/. The main deltas are:

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 )

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.
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@zizifn
Copy link

zizifn commented Jul 12, 2022

I think webkit has feature bug already? https://bugs.webkit.org/show_bug.cgi?id=220823

@allstarschh
Copy link

@domenic
Could you explain when the preparation-time document check will fail for import-maps?
As the import map doesn't support external, neither it supports 'async' nor 'defer',
the 'execute the script element' part should be run immediately after 'prepare the script element' for inline import maps, so the 'preparation-time document check' should always succeed.
Should the 'register an import map' be done in 'prepare the script element' ?

@domenic
Copy link
Member Author

domenic commented Jul 27, 2022

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.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@hiroshige-g
Copy link
Contributor

Sorry for delay, LGTM. My comments are editorial except for #8075 (comment).

@domenic domenic requested a review from annevk August 3, 2022 07:31
@domenic
Copy link
Member Author

domenic commented Aug 3, 2022

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.

Copy link
Member

@annevk annevk left a 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 Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
Comment on lines 95432 to 95434
<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>
Copy link
Member

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.

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 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.)

Copy link
Member

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.

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 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.

Copy link
Member

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.

Copy link
Member Author

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.?

Copy link
Member

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!

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk annevk mentioned this pull request Oct 5, 2022
2 tasks
Copy link
Member

@annevk annevk left a 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.

@domenic domenic merged commit 1e55204 into task-source-for-script Oct 5, 2022
@domenic domenic deleted the importmaps branch October 5, 2022 07:26
@dontcallmedom
Copy link
Contributor

dontcallmedom commented Oct 5, 2022

is it intended it got merged into task-source-for-script rather than main?

@domenic
Copy link
Member Author

domenic commented Oct 5, 2022

Nope, sorting that out now over in #8072.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: script
Development

Successfully merging this pull request may close these issues.

7 participants