Closed Bug 1342478 Opened 3 years ago Closed 3 years ago

Support relative paths in shell module loader

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached 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.
Attached patch bug1342478.patchSplinter Review
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.