Closed
Bug 1215063
Opened 9 years ago
Closed 9 years ago
Support modules in the shell
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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 embedjs.py script.
Attachment #8674939 -
Flags: review?(shu)
Assignee | ||
Comment 3•9 years ago
|
||
Add support for jit-tests to run a test as a module and add some tests.
Attachment #8674940 -
Flags: review?(shu)
Comment 4•9 years ago
|
||
Comment on attachment 8674933 [details] [diff] [review]
shell-1-path-support
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 5•9 years ago
|
||
Comment on attachment 8674939 [details] [diff] [review]
shell-2-shell-module-support
Review of attachment 8674939 [details] [diff] [review]:
-----------------------------------------------------------------
Looks fine. Could you ensure you didn't blow away other Reflect stuff?
::: js/src/shell/Makefile.in
@@ +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 http://mozilla.org/MPL/2.0/. */
> +
> +// 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 6•9 years ago
|
||
Comment on attachment 8674939 [details] [diff] [review]
shell-2-shell-module-support
This will need build peer review
Updated•9 years ago
|
Attachment #8674940 -
Flags: review?(shu) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8674939 [details] [diff] [review]
shell-2-shell-module-support
Requesting build peer review for changes to makefiles etc.
Attachment #8674939 -
Flags: review?(ted)
Comment 8•9 years ago
|
||
Comment on attachment 8674939 [details] [diff] [review]
shell-2-shell-module-support
Review of attachment 8674939 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/shell/Makefile.in
@@ +51,5 @@
> +
> +MODULELOADER_DEFINES += $(DEFINES) $(ACDEFINES) $(MOZ_DEBUG_DEFINES)
> +
> +moduleloader.out.h: $(moduleloader_out_h_deps)
> + $(PYTHON) $(srcdir)/../builtin/embedjs.py $(MODULELOADER_DEFINES) \
You should be able to write this using GENERATED_FILES in moz.build nowadays:
https://dxr.mozilla.org/mozilla-central/rev/b41b92c09fcf94d077a54297aea1dc675b161a9d/dom/base/moz.build#477
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)
Assignee | ||
Comment 9•9 years ago
|
||
Updated patch following review comments and embedjs script refactoring in bug 1220731.
Attachment #8674939 -
Attachment is obsolete: true
Attachment #8684236 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8684236 [details] [diff] [review]
shell-2-shell-module-support
Requesting build peer review for moz.build and embedjs.py changes.
Attachment #8684236 -
Flags: review?(nfroyd)
Comment 11•9 years ago
|
||
Comment on attachment 8684236 [details] [diff] [review]
shell-2-shell-module-support
Review of attachment 8684236 [details] [diff] [review]:
-----------------------------------------------------------------
Seems reasonable to me.
::: js/src/builtin/embedjs.py
@@ +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+
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/548a09b1a4e6
https://hg.mozilla.org/mozilla-central/rev/26f84ff40db5
https://hg.mozilla.org/mozilla-central/rev/e3d2cb194e98
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•