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

Use "queue an element task" properly in script preparation #8072

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 4, 2022

Previously the task had no specified task source.

Because task sources are not very observable, this is mostly good spec hygiene.

  • At least two implementers are interested (and none opposed):
    • Probably this is fine? Task sources are not very testable in general. By code inspection, Gecko and WebKit use some sort of "run on the main thread" call (WebKit might fire these sync in some cases??). Chromium uses the "DOM manipulation task source" which is why I chose that one, instead of e.g. the networking task source.
  • Tests are written and can be reviewed and commented upon at:
    • I don't think we need tests here, since task source ordering is not really testable. Maybe we could do basic tests that these are fired async instead of sync, but that would be more good-citizenship-while-we're-in-the-area tests, not testing this change in particular
  • Implementation bugs are filed:
    • Chrome: N/A
    • Firefox: N/A I think
    • Safari: N/A I think
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …

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

Previously the task had no specified task source.
@domenic
Copy link
Member Author

domenic commented Oct 5, 2022

lol I forgot I had targeted the import maps spec here... @annevk can you approve, based on the first commit? I think I have a couple merge conflicts to fix too, but I'll work on that...

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.

First commit looks great!

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. See #8355.

* 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#268, WICG/import-maps#270, WICG/import-maps#276, WICG/import-maps#277.
@domenic domenic merged commit ef77849 into main Oct 5, 2022
@domenic domenic deleted the task-source-for-script branch October 5, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants