feat(endo): Add CommonJS and JSON module support #397

kriskowal · 2020-07-31T00:33:46Z

Achieves parity with Node.js behavior regarding the "type": "module" declaration in package.json, determining whether .js files are CommonJS or ECMAScript modules. Also treats the "module" declaration as a hint that the denoted module is an ESM, regardless of its extension.

The emulation of Node.js behavior is not exact.

In Node.js, ECMAScript modules can use require to import CommonJS modules, but cannot use import from CommonJS. In Endo.js, an ESM can only use import and a CJS module can only use require, but both can be used to import any supported module including ECMAScript modules, CommonJS modules, and JSON modules.
In Node.js, a CommonJS script cannot use require to import a CommonJS. module. In Endo.js, they can.
The Endo.js convention for ESM importing CJS is that require will return the default export if it exists and assigning to module.exports is equivalent to assigning to exports.default. This may break existing CommonJS code in the rare case that has an export named default. Consequently, ESM and CJS destructuring imports are the same.
This works because Endo.js uses the same heuristics as build-tools for CommonJS imports. The require calls must be exclusively static, meaning the argument must be a string and all imports are unconditional.
Of course, Endo.js only works for modules within packages. Imports cannot reach arbitrarily outward into the filesystem.
Node.js ESM cannot use import to load a JSON module. Endo.js can import JSON into any module. The module formats are orthogonal.

kriskowal · 2020-07-31T00:35:20Z

cc @kumavis This came together faster than expected.

kriskowal · 2020-08-01T03:25:08Z

Replaced the whacky regex with a proper Babel visitor, so static analysis for require(id) calls is now as accurate as it will likely get.

warner · 2020-08-05T19:30:46Z

Could you rebase this when you get a chance? The diff currently shows all the endo-main changes too, which should go away when the branch is moved to sit properly on top of the updated target.

kriskowal · 2020-08-06T18:24:17Z

Could you rebase this when you get a chance? The diff currently shows all the endo-main changes too, which should go away when the branch is moved to sit properly on top of the updated target.

Rebased this stack.

warner

minor changes. looks good!

warner · 2020-08-22T21:27:26Z

packages/endo/test/parse-requires.test.js

+test("parse unique require calls", t => {
+  t.plan(1);
+  const code = `
+    require("b"); // sorted later


If the code were failing to find inner-scope'd require, would this second occurrence of require("b") make this test fail to find the bug? I.e does this occurrence nullify the negative predictive value of the test?

also, maybe this test should exercise a space betwen require and ( , and perhaps a newline, if those are things that Node will accept

eh, probably not worth it, we are using a proper parser after all. (back in my day, we made do with regexps, and I would have been worried about something like this. Six feet of snow, uphill both ways, yadda yadda)

There’s a commit with the regex version in this very stack, borrowed from my back-in-the-day implementation ❤️

warner · 2020-08-24T00:46:21Z

packages/endo/src/lockdown.js

@@ -0,0 +1,6 @@
+/* global lockdown */


this file doesn't appear to be used (yet)?

warner · 2020-08-24T00:51:38Z

packages/endo/src/parse-requires.js

+ * the module execution phase.
+ * For example, `require(path)` will not be discovered by this parser, the
+ * module will successfully load, but will likely be unable to synchronously
+ * require the module with the given path.


in Jetpack, our recommendation for this was to combine the two effects:

if (false) { require('thing1'); } if (false) { require('thing2'); } ... const needed = condition ? 'thing1' : 'thing2'; require(needed);

.. but really, if you're reaching for something like that, there's probably something deeper going on, and you're going to get into trouble anyways.

warner · 2020-08-24T01:22:37Z

packages/endo/src/parse.js

+  }
+
+  const imports = parseRequires(source, location);
+  const execute = (exports, compartment, resolvedImports) => {


Curious, why is exports passed into execute() when it seems the only properties on it come from the module being loaded? I would have expected execute() to return exports instead. Is this some quirk of the new module loader specification?

CommonJS has always supported cyclic dependencies in this fashion. Two modules can mutually require as long as they lazy-bind properties of the exports returned by require, and as long as they use exports.property = assignment instead of module.exports = replacement. The module.exports property was not part of the original CommonJS, although it has become dominant usage in Node.js. The module object originally existed in CommonJS to fulfill the same function as import.meta.

warner · 2020-08-24T01:33:57Z

packages/endo/src/parse.js

+};
+
+const parserForLanguage = {
+  mjs: parseMjs,


Any thoughts about using "esm" for the name of the language, rather than "mjs"? I was puzzled for a while until I realized this was not an extension name.

As you noted later, this becomes coherent with the "parsers" directive in package.json that we need in order to break the log-jab between Node.js and -r ESM’s interpretation of the "type": "module" directive. I felt that translating between "esm" in the domain and "mjs" in the range for just this one format may have been more confusing.

warner · 2020-08-24T01:54:44Z

packages/endo/test/parse-requires.test.js

+    require("a");
+
+    function b() {
+      require("b"); // found despite inner scope


I think this needs a different name (maybe d) to test inner scopes without interference from the same-named require('b') on line 9. If the parser had a bug in which inner scopes were ignored, this test would yield a false positive.

warner · 2020-08-24T01:56:52Z

packages/endo/test/parse-requires.test.js

+    require("a"); // de-duplicated
+
+    require("id\\""); // found, despite inner quote
+  `;


you might consider a test that require('e', otherarg) is ignored, since that's known behavior of the parser. OTOH I'd understand if you wanted to omit it, since ignoring multi-arg require is not really part of the spec.

warner · 2020-08-24T02:01:11Z

packages/endo/src/compartmap.js

@@ -70,6 +73,21 @@ const findPackage = async (readDescriptor, directory, name) => {
  }
 };

+const commonParsers = { js: "cjs", cjs: "cjs", mjs: "mjs", json: "json" };
+const moduleParsers = { js: "mjs", mjs: "mjs", cjs: "cjs", json: "json" };


for the sake of visual comparison, these should both have their keys in the same order, rather than swapping cjs/mjs between the two

nevermind, I see this issue goes away in another couple of PRs

Achieves parity with Node.js behavior regarding the "type": "module" declaration in package.json, determining whether `.js` files are CommonJS or ECMAScript modules. Goes beyond parity to support JSON modules.

kriskowal requested a review from warner July 31, 2020 00:33

kriskowal force-pushed the kris/endo-main branch from 52722d0 to 7c9aab3 Compare July 31, 2020 00:36

kriskowal force-pushed the kris/endo-cjs branch from 2a7d008 to 26c260b Compare July 31, 2020 00:36

kriskowal linked an issue Jul 31, 2020 that may be closed by this pull request

Compartment support for CommonJS #297

Closed

kriskowal force-pushed the kris/endo-main branch from 7c9aab3 to a6f6bc3 Compare July 31, 2020 02:19

kriskowal force-pushed the kris/endo-cjs branch from 26c260b to 1838f01 Compare July 31, 2020 02:19

kriskowal force-pushed the kris/endo-main branch from a6f6bc3 to e1ad2fd Compare July 31, 2020 15:36

kriskowal force-pushed the kris/endo-cjs branch from 1838f01 to 94de2dc Compare July 31, 2020 15:36

kriskowal force-pushed the kris/endo-main branch from e1ad2fd to c9ebbbb Compare August 1, 2020 03:13

kriskowal force-pushed the kris/endo-cjs branch from 94de2dc to 0cebbf5 Compare August 1, 2020 03:13

kriskowal mentioned this pull request Aug 3, 2020

Using XS native Compartments for Agoric Vat Workers #400

Open

29 tasks

kriskowal force-pushed the kris/endo-cjs branch from 0cebbf5 to c85bc45 Compare August 3, 2020 23:28

kriskowal force-pushed the kris/endo-main branch from 76d153f to 9d6b9b1 Compare August 5, 2020 01:10

kriskowal force-pushed the kris/endo-cjs branch from c85bc45 to 84f4b2b Compare August 5, 2020 01:10

kriskowal force-pushed the kris/endo-main branch from 9d6b9b1 to e749986 Compare August 5, 2020 01:22

kriskowal force-pushed the kris/endo-cjs branch from 84f4b2b to ffb07e0 Compare August 6, 2020 16:17

kriskowal force-pushed the kris/endo-main branch from d73fdd1 to b98aa35 Compare August 6, 2020 18:15

kriskowal force-pushed the kris/endo-cjs branch from ffb07e0 to 9872570 Compare August 6, 2020 18:15

kriskowal mentioned this pull request Aug 6, 2020

Endo #336

Closed

36 tasks

kriskowal force-pushed the kris/endo-main branch from b98aa35 to f2bb4fb Compare August 7, 2020 22:19

kriskowal force-pushed the kris/endo-cjs branch from 9872570 to 8010098 Compare August 7, 2020 22:19

kriskowal force-pushed the kris/endo-main branch from f2bb4fb to fcd85c2 Compare August 7, 2020 22:43

kriskowal force-pushed the kris/endo-cjs branch 2 times, most recently from 9d1453c to 8e6f345 Compare August 10, 2020 22:44

kriskowal force-pushed the kris/endo-main branch from 61ad42e to edf01ad Compare August 11, 2020 00:03

kriskowal force-pushed the kris/endo-cjs branch from 8e6f345 to 12716a6 Compare August 11, 2020 00:03

kriskowal force-pushed the kris/endo-cjs branch from 12716a6 to 9698520 Compare August 11, 2020 00:07

kriskowal force-pushed the kris/endo-main branch from 1d2c4a8 to 8a2e109 Compare August 12, 2020 00:31

kriskowal force-pushed the kris/endo-cjs branch from 9698520 to 12d4ec5 Compare August 12, 2020 00:31

kriskowal force-pushed the kris/endo-main branch from 8a2e109 to cbb7efc Compare August 12, 2020 00:32

kriskowal force-pushed the kris/endo-cjs branch 2 times, most recently from 1e1a19c to e4ef2b7 Compare August 12, 2020 00:35

kriskowal force-pushed the kris/endo-cjs branch from e4ef2b7 to 2c96932 Compare August 22, 2020 01:32

Base automatically changed from kris/endo-main to master August 22, 2020 19:12

kriskowal force-pushed the kris/endo-cjs branch 2 times, most recently from 165bf77 to 4ff24fb Compare August 22, 2020 20:45

warner approved these changes Aug 24, 2020

View reviewed changes

kriskowal added 16 commits August 24, 2020 12:03

feat(endo): Add CommonJS and JSON module support

2ce0533

Achieves parity with Node.js behavior regarding the "type": "module" declaration in package.json, determining whether `.js` files are CommonJS or ECMAScript modules. Goes beyond parity to support JSON modules.

endo-cjs: Lint fix

e2e60c9

Replace require parser heuristic with proper parser

0152d12

Freeze CommonJS module object

860270c

Favor final module extension segment

b1e557c

Add parse-requires and test

682f248

Move parseExtension

3750750

Lint parseExtension

da71c3d

Finish renaming extension parser module

3726cd9

Annotate CommonJS parse error location

c100e33

Document CommonJS and ESM interportability

8e9a6a6

Document JSON module support

1cebfc8

Lint: remove parseExtension from archive

d75398f

Update yarn.lock

231d16f

Improve parseRequires test

621bd54

Remove lockdown.js (too soon)

cd6b87d

kriskowal force-pushed the kris/endo-cjs branch from 4ff24fb to cd6b87d Compare August 24, 2020 19:03

kriskowal merged commit fe927df into master Aug 24, 2020

kriskowal deleted the kris/endo-cjs branch August 24, 2020 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(endo): Add CommonJS and JSON module support #397

feat(endo): Add CommonJS and JSON module support #397

kriskowal commented Jul 31, 2020 •

edited

Loading

kriskowal commented Jul 31, 2020

kriskowal commented Aug 1, 2020

warner commented Aug 5, 2020

kriskowal commented Aug 6, 2020

warner left a comment

warner Aug 22, 2020

warner Aug 22, 2020

warner Aug 24, 2020

kriskowal Aug 24, 2020

warner Aug 24, 2020

warner Aug 24, 2020

warner Aug 24, 2020

kriskowal Aug 24, 2020

warner Aug 24, 2020

kriskowal Aug 24, 2020

warner Aug 24, 2020

warner Aug 24, 2020

warner Aug 24, 2020

warner Aug 24, 2020

feat(endo): Add CommonJS and JSON module support #397

feat(endo): Add CommonJS and JSON module support #397

Conversation

kriskowal commented Jul 31, 2020 • edited Loading

kriskowal commented Jul 31, 2020

kriskowal commented Aug 1, 2020

warner commented Aug 5, 2020

kriskowal commented Aug 6, 2020

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Jul 31, 2020 •

edited

Loading