1342478 - Support relative paths in shell module loader

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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.

Comment 1

[André Bargull [:anba]]

Attachment: bug1342478.patch

Adds a module->path map to Reflect.Loader, so we can compute relative paths in Reflect.Loader.resolve().


Applies on top of bug 1340146.

Comment 2

[Jon Coppeard (:jonco)]

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?

Comment 3

[André Bargull [:anba]]

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

Comment 4

[André Bargull [:anba]]

Attachment: bug1342478.patch

Updated patch per review comments, carrying r+ from jonco.

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a345af03b454db2f5bdc576a4d322a44e95a92

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd83ad75120
Support loading modules from relative paths in shell module loader. r=jonco

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2cd83ad75120