bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Support relative paths in shell module loader

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8840977 [details] [diff] [review]
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.
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+
(Assignee)

Comment 3

a year ago
(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.
(Assignee)

Comment 4

a year ago
Created attachment 8842816 [details] [diff] [review]
bug1342478.patch

Updated patch per review comments, carrying r+ from jonco.
Attachment #8840977 - Attachment is obsolete: true
Attachment #8842816 - Flags: review+

Comment 6

a year ago
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

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2cd83ad75120
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.