Support relative paths in shell module loader

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The shell module loader doesn't support to load modules relative to another module, it always loads modules relative to the module-load-path. This is an issue for test262 module tests, which use relative paths in import statements. As a workaround I'm currently setting the --module-load-path shell option to the directory of the main module (bug 1340583, patch part 4), but I'd prefer to have proper support for relative paths in the shell module loader.
Posted patch bug1342478.patch (obsolete) — Splinter Review
Adds a module->path map to Reflect.Loader, so we can compute relative paths in Reflect.Loader.resolve().


Applies on top of bug 1340146.
Attachment #8840977 - Flags: review?(jcoppeard)
Comment on attachment 8840977 [details] [diff] [review]
bug1342478.patch

Review of attachment 8840977 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for fixing these.

::: js/src/shell/ModuleLoader.js
@@ +31,5 @@
> +            // Treat |name| as a relative path if it starts with either "./"
> +            // or "../".
> +            let isRelative = false;
> +            if (name.length > 1 && name[0] === ".") {
> +                let sl = name[1] === "." && name.length > 2 ? 2 : 1;

It may be simpler to use startsWith.

@@ +41,5 @@
> +            }
> +
> +            // If |name| is a relative path and |module|'s path is available,
> +            // load |name| relative to the referring module.
> +            if (isRelative && ReflectApply(MapPrototypeHas, this.modulePaths, [module])) {

Is there any situation where module is non-null but we don't have a path?
Attachment #8840977 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #2)
> Review of attachment 8840977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks for fixing these.
> 

Thanks for reviewing! :-)

> ::: js/src/shell/ModuleLoader.js
> @@ +31,5 @@
> > +            // Treat |name| as a relative path if it starts with either "./"
> > +            // or "../".
> > +            let isRelative = false;
> > +            if (name.length > 1 && name[0] === ".") {
> > +                let sl = name[1] === "." && name.length > 2 ? 2 : 1;
> 
> It may be simpler to use startsWith.

Agreed, I'll update the patch. 


> 
> @@ +41,5 @@
> > +            }
> > +
> > +            // If |name| is a relative path and |module|'s path is available,
> > +            // load |name| relative to the referring module.
> > +            if (isRelative && ReflectApply(MapPrototypeHas, this.modulePaths, [module])) {
> 
> Is there any situation where module is non-null but we don't have a path?

I think that's only possible when parseModule() and module.declarationInstantiation() are called manually.
Updated patch per review comments, carrying r+ from jonco.
Attachment #8840977 - Attachment is obsolete: true
Attachment #8842816 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd83ad75120
Support loading modules from relative paths in shell module loader. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2cd83ad75120
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.