Closed Bug 1778289 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

Attachments

(12 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Below is the list of things changed:

Severity: -- → S4
Priority: -- → P3
Summary: Update Import-Maps implementation to meet the latest whatwg PR → Update Import-Maps implementation to meet the latest whatwg PR 8075

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

Below is the list of things changed:

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

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.

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

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.

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

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

Attachment #9287228 - Attachment description: Bug 1778289 - Part 3: Update spec links in ModuleLoaderBase::ParseImportMap → Bug 1778289 - Part 3: Update spec links in ModuleLoaderBase::ParseImportMap.
Attachment #9287232 - Attachment description: Bug 1778289 - Part 7: Update ImportMaps.cpp/h. → Bug 1778289 - Part 7: Update spec in ImportMaps.cpp/h.
Attachment #9287226 - Attachment description: Bug 1778289 - Part 1: Resolve a URL-like module speicier. → Bug 1778289 - Part 1: Rename 'parse a URL-Like import specifier' to 'resolve a URL-Like module specifier'.
Attachment #9287227 - Attachment description: Bug 1778289 - Part 2: Disallow import maps. → Bug 1778289 - Part 2: Rename to 'import maps allowed' and add 'disallow import maps'.

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

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.

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.

Flags: needinfo?(ystartsev)
Flags: needinfo?(allstars.chh)

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

Flags: needinfo?(ystartsev)
Flags: needinfo?(allstars.chh)

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.

Pushed by allstars.chh@gmail.com:
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
Depends on: 1801764
No longer depends on: 1801764
You need to log in before you can comment on or make changes to this bug.