Skip to content

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

Merged
domenic merged 2 commits intomainfrom
task-source-for-script
Oct 5, 2022
Merged

Use "queue an element task" properly in script preparation#8072
domenic merged 2 commits intomainfrom
task-source-for-script

Conversation

@domenic
Copy link
Copy Markdown
Member

@domenic domenic commented Jul 4, 2022 •
edited by pr-preview bot
Loading

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 )

@domenic domenic requested a review from hiroshige-g July 4, 2022 09:55
@domenic
Use "queue an element task" properly in script preparation
7d67a88 
Previously the task had no specified task source.
@domenic
Copy link
Copy Markdown
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...

annevk
annevk approved these changes Oct 5, 2022
View reviewed changes
Copy link
Copy Markdown
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!

@domenic domenic force-pushed the task-source-for-script branch from 1e55204 to 669c64c Compare October 5, 2022 07:46
@domenic domenic mentioned this pull request Oct 5, 2022
Merged
3 tasks
@domenic domenic force-pushed the task-source-for-script branch from 669c64c to 84108ac Compare October 5, 2022 07:49
@domenic
Add basic import maps support
dacf8df 
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 force-pushed the task-source-for-script branch from 84108ac to dacf8df Compare October 5, 2022 07:55
@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

Reviewers

@annevk annevk annevk approved these changes

@hiroshige-g hiroshige-g Awaiting requested review from hiroshige-g

Assignees

No one assigned

Labels

topic: event loop topic: script

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@domenic @annevk