Closed Bug 1215063 Opened 8 years ago Closed 8 years ago

Support modules in the shell


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: jonco, Assigned: jonco)


(Blocks 1 open bug)



(3 files, 1 obsolete file)

The goal of this bug is to add support for ES2015 modules in the JS shell.

There will only be support for a very basic module loader.  The shell doesn't support promises so everything will be done synchronously.
Add support in the shell for some basic utilities to manipulate pathnames which are needed for the shell module loader:

os.path.isAbsolute() - determine whether a patch is absolute
os.path.join() - join one or more path components together

join() doesn't do all the sophisticated things that pythons os.path.join() does on Windows systems (I'm taking that as my reference because I don't know how things should work on Windows anyway).  If you think this is important I can add them though.
Attachment #8674933 - Flags: review?(sphink)
Attached patch shell-2-shell-module-support (obsolete) — Splinter Review
This adds a basic module loader and shell support for running a module script from the command line by passing the -m or --module options.

The module loader interprets module names that are absolute paths as filenames, and anything as as a path relative to a module load path that can be passed as an option to the shell.

The module loader is embedded in the shell in the same way as we do for selfhosted code, by using the script.
Attachment #8674939 - Flags: review?(shu)
Attached patch shell-3-testsSplinter Review
Add support for jit-tests to run a test as a module and add some tests.
Attachment #8674940 - Flags: review?(shu)
Comment on attachment 8674933 [details] [diff] [review]

Review of attachment 8674933 [details] [diff] [review]:

Good enough for me. I played around with the shell in Windows, and I can't even figure out what paths it wants when. I think the "right" solution would be to use PathCchCombine when XP_WIN, except that doesn't exist in Windows XP, so you'd have to use PathCombine, and... it just seems like too much trouble. But when this causes issues for somebody, that'd be the fix.
Attachment #8674933 - Flags: review?(sphink) → review+
Comment on attachment 8674939 [details] [diff] [review]

Review of attachment 8674939 [details] [diff] [review]:

Looks fine. Could you ensure you didn't blow away other Reflect stuff?

::: js/src/shell/
@@ +36,5 @@
>  	$(SYSINSTALL) $^ $(DESTDIR)$(bindir)
> +
> +# Prepare module loader code for embedding
> +export:: moduleloader
> +moduleloader:: moduleloader.out.h

Could you rename this shellmoduleloader or something that specifically says it's just for the shell?

::: js/src/shell/ModuleLoader.js
@@ +3,5 @@
> + * file, You can obtain one at */
> +
> +// A basic synchronous module loader for testing the shell.
> +
> +Reflect = {

What about the existing Reflect object in the shell?
Attachment #8674939 - Flags: review?(shu) → review+
Comment on attachment 8674939 [details] [diff] [review]

This will need build peer review
Attachment #8674940 - Flags: review?(shu) → review+
Comment on attachment 8674939 [details] [diff] [review]

Requesting build peer review for changes to makefiles etc.
Attachment #8674939 - Flags: review?(ted)
Comment on attachment 8674939 [details] [diff] [review]

Review of attachment 8674939 [details] [diff] [review]:

::: js/src/shell/
@@ +51,5 @@
> +
> +
> +moduleloader.out.h: $(moduleloader_out_h_deps)
> +	$(PYTHON) $(srcdir)/../builtin/ $(MODULELOADER_DEFINES) \

You should be able to write this using GENERATED_FILES in nowadays:

You'll probably have to either modify the Python script slightly (the build system passes in some info to a specific Python function) or write a little new Python script that calls into the existing script the right way, but it should avoid you having to write a bunch of new Makefile rules.

Can you try that out and see if it works? If you run into problems ask in #build and hopefully we can get it sorted.
Attachment #8674939 - Flags: review?(ted)
Depends on: 1220731
Updated patch following review comments and embedjs script refactoring in bug 1220731.
Attachment #8674939 - Attachment is obsolete: true
Attachment #8684236 - Flags: review+
Comment on attachment 8684236 [details] [diff] [review]

Requesting build peer review for and changes.
Attachment #8684236 - Flags: review?(nfroyd)
Comment on attachment 8684236 [details] [diff] [review]

Review of attachment 8684236 [details] [diff] [review]:

Seems reasonable to me.

::: js/src/builtin/
@@ +94,5 @@
>      'sources_data': data,
>      'sources_name': 'compressedSources',
>      'compressed_total_length': len(compressed),
> +    'raw_total_length': len(processed),
> +        'namespace': namespace

Nit: the indentation seems off here.
Attachment #8684236 - Flags: review?(nfroyd) → review+
No longer depends on: 1325551
You need to log in before you can comment on or make changes to this bug.