1778289 - Update Import-Maps implementation to meet the latest whatwg PR 8075

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/whatwg/html/pull/8075

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Below is the list of things changed:

• Parse a URL-like import specifier has been renamed to Resolve a URL-like module specifier.
• Register an import map has been updated, as the script's result will be import map parse result, and also now it's called during execute the script element (Previously it was called during prepare a script).
• Acquiring Import Maps has been renamed to Import Maps allowed, with the new added method: Disallow further Import Maps.
• Resolve a module specifier now takes a script-or-null and a string parameters, and will throw a TypeError if it fails. Originally it takes a URL and a string. See Original Resolve a module specifier. This also changes the callers of the resolve-a-module-specifier. Specifically when Create a Module Script, the exception will be set to the parse error(Step 10.2).

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #1)

Below is the list of things changed:

• Register an import map has been updated, as the script's result will be import map parse result, and also now it's called during execute the script element (Previously it was called during prepare a script).

As the register an import map now moves to execute the script element, I'll try to add a ImportMapLoadRequest to store the parsed import map (script's result from spec), and then register the import map parsed result in EvaluateScriptElement

• Resolve a module specifier now takes a script-or-null and a string parameters, and will throw a TypeError if it fails. Originally it takes a URL and a string. See Original Resolve a module specifier. This also changes the callers of the resolve-a-module-specifier. Specifically when Create a Module Script, the exception will be set to the parse error(Step 10.2).

From the spec,

For each ModuleRequest record requested of result.[[RequestedModules]]:
    Let url be the result of resolving a module specifier given script and requested.[[Specifier]], catching any exceptions.

The tricky part is the "catching any exceptions", I'll check if we should address that or leave as it is now.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #2)

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #1)

Below is the list of things changed:

• Register an import map has been updated, as the script's result will be import map parse result, and also now it's called during execute the script element (Previously it was called during prepare a script).

As the register an import map now moves to execute the script element, I'll try to add a ImportMapLoadRequest to store the parsed import map (script's result from spec), and then register the import map parsed result in EvaluateScriptElement

As explained by the spec author: https://github.com/whatwg/html/pull/8075#issuecomment-1196276156
the preparation-time document check will never fail for import maps.
For now we can ignore this spec change, I'll file another bug to address that if we need to register the import map when executing the script element.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 1: Rename 'parse a URL-Like import specifier' to 'resolve a URL-Like module specifier'.

https://whatpr.org/html/8075/webappapis.html#resolving-a-url-like-module-specifier

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 2: Rename to 'import maps allowed' and add 'disallow import maps'.

https://whatpr.org/html/8075/webappapis.html#import-maps-allowed
https://whatpr.org/html/8075/webappapis.html#disallow-further-import-maps

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 3: Update spec links in ModuleLoaderBase::ParseImportMap.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 4: Don't call onerror if parsing import maps fails.

script's result will be an import map parse result, which will never be
null.

See explanation from https://github.com/WICG/import-maps/issues/279

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 5: Update spec in RegisterImportMap.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 6: Update comments for external import maps.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 7: Update spec in ImportMaps.cpp/h.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 : Part 8: Update spec for prepare-script when type="importmap".

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 9: Bail out early if adding an import map is disallowed.

If an adding import map is disallowed, we bail out early to prevent creating a
ScriptLoadRequest.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 10: Update comments for 'register an import map'.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

As Jonco mentioned the spec link is still from the PR (The link is still https://whatpr.org/html/8075/...)
https://phabricator.services.mozilla.com/D152870#inline-843162

As I have no idea when the PR will be merged, so I wrote these patches with the links from the PR.
Once these patches are all got accepted, I'll leave a comment in the PR to notify that the patches are accepted.
We'll wait until the PR is merged, then I'll have a separate patch to update the links and land these patches

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 11: Update comments for ResolveModuleSpecifier.

According to the spec, the method should throw a TypeError if it fails
to resolve the specifier. Here we add some comments to explain why we
return a result code instead of throwing an error.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:allstars.chh, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

We are still waiting for the PR to be merged, as it might be updated during the review process.
https://github.com/whatwg/html/pull/8075

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

The PR is merged yesterday.
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps

I'll rebase my patches and update the spec link.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1778289 - Part 12: Update spec links as the PR is merged.

The PR has been merged.
See https://html.spec.whatwg.org/multipage/webappapis.html#import-maps

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88647780854a
Part 1: Rename 'parse a URL-Like import specifier' to 'resolve a URL-Like module specifier'. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/b68b8f0d608c
Part 2: Rename to 'import maps allowed' and add 'disallow import maps'. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/3e2e1fbc651a
Part 3: Update spec links in ModuleLoaderBase::ParseImportMap. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/829f0f41f7fc
Part 4: Don't call onerror if parsing import maps fails. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/6489cf84de36
Part 5: Update spec in RegisterImportMap. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/3e1a89dcc16c
Part 6: Update comments for external import maps. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/03257f7c2f05
Part 7: Update spec in ImportMaps.cpp/h. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/69c1c66dd528
Part 8: Update spec for prepare-script when type="importmap". r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/b17fa8f361f1
Part 9: Bail out early if adding an import map is disallowed. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/fcd66d7f4917
Part 10: Update comments for 'register an import map'. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/4a1dc4785996
Part 11: Update comments for ResolveModuleSpecifier. r=jonco,yulia
https://hg.mozilla.org/integration/autoland/rev/36d3f80544ec
Part 12: Update spec links as the PR is merged. r=jonco,yulia

Comment 21

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/88647780854a
https://hg.mozilla.org/mozilla-central/rev/b68b8f0d608c
https://hg.mozilla.org/mozilla-central/rev/3e2e1fbc651a
https://hg.mozilla.org/mozilla-central/rev/829f0f41f7fc
https://hg.mozilla.org/mozilla-central/rev/6489cf84de36
https://hg.mozilla.org/mozilla-central/rev/3e1a89dcc16c
https://hg.mozilla.org/mozilla-central/rev/03257f7c2f05
https://hg.mozilla.org/mozilla-central/rev/69c1c66dd528
https://hg.mozilla.org/mozilla-central/rev/b17fa8f361f1
https://hg.mozilla.org/mozilla-central/rev/fcd66d7f4917
https://hg.mozilla.org/mozilla-central/rev/4a1dc4785996
https://hg.mozilla.org/mozilla-central/rev/36d3f80544ec