Closed Bug 1778289 Opened 3 years ago Closed 3 years 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.

Attachment

General

Created:
Updated:
Size: