use Node's ES-module support, stop using -r esm

[warner]

Node.js now has enough support for real ES modules, so we can stop using the esm module. I've started doing this cleanup for SwingSet, but we should do it across the repo. The requirements are:

• package.json must have type: "module"
• the main: entry in package.json must point to a module-style file (with an export statement), not a CJS-style file (with exports.foo = or module.exports = )
• we should remove esm from the package.json devDependencies and dependencies
• remove -r esm from invocations of node or other tools from package.json and other scripts
• in all code: relative imports (import xx from './foo' or import xx from '../bar/blah') must be expanded to name the exact file: import xx from './foo.js'. Node's ESM does not search x, x.js, and x/index.js the way it does for CommonJS.
• 🆕 All uses of require.resolve(path), for a path like path.join(__dirname, path), need to be replaced with new URL(path, import.meta.url).pathname
• 🆕 All uses of require.resolve(specifier), for a specifier like @agoric/zoe/contractFacet.js, need to be replaced by importMetaResolve(specifier, import.meta.url) and use the import-meta-url package from npm. In the cases where these are being used to normalize a specifier within a config file, this is an opportunity to fix a bug by replacing import.meta.url with the URL of the config file, e.g., new URL(configPath, import.meta.url).toString(). Note that importMetaResolve is asynchronous so some workflows may need to be transformed to async functions.
• 🆕 Use import url from 'url' and url.filePathToURL(await importMetaResolve(moduleSpecifier, import.meta.url)) to get the path for a module specifier, as with a contract path when passed to bundleSource. As a matter of style and safety, we only use await as a statement at the topmost level of an async function, so be sure to expand this example to two lines with an intermediate named variable. The url.filePathToURL function is portable even unto Windows.
• 🆕 All uses of __dirname and __filename must also be migrated, using new URL('./', import.meta.url).pathname and new URL(import.meta.url).pathname respectively, if not one of the above patterns. Be sure to remove these from the /* global */ directive and you may also need to remove the import path from 'path' statement to appease the linter.
• 🆕 .jsconfig.json needs a "module": "esnext" in addition to "target": "esnext" to willingly interpret import.meta.url.
• 🆕 The parsers directive in package.json, if present, may be removed. This exists as a work-around for portability straddling RESM and SESM and is not necessary to straddle SESM and NESM.
• 2️⃣ With NESM, every module must have an extension, including binaries. However, binaries can be symbolic links to sources with extensions. With RESM, we used a variety of techniques, like having a bin/x thunk that would require('esm')('../src/entrypoint.js'), since entrypoint modules still had to be CommonJS format.

Any package imported by the Node module logic must have type: "module" in its package.json, else it won't be recognized as importable, so we basically have to make this change from the bottom of the dependency tree, working our way up.

🆕 This now blocks security blocker endojs/endo#850

[katelynsills]

This sounds great. I suspect we will have to change the eslint-plugin-import/extensions rule to always: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/extensions.md

[warner]

It looks like we may also have to use tap instead of tape to run all our tests, since tape seems to use require() to import the file files, and that is no longer allowed within Node's native module support.

@jfparadis does that seem right? I know you've been using tap from the start on SES-beta, so if that's the way to go, I'm happy to have agoric-sdk follow your lead.

[jfparadis]

Correct. There are 2 things in tests:

1. The test library (what you import in your test code)
2. The test runner (the cli you run)

It's perfectly fine to use tape in a test and tap as the runner. That's what I'm using in ses-beta, and indicated in a chat today.

tape isn't designed to load ESM module, only CJS:

    files.forEach(function (file) {
        require(resolvePath(cwd, file));
    });

tape itself works fine when referenced from an ESM module executing with native ESM module support, but it uses the "require" syntax to load test files.

[warner]

It's starting to look like we have to start from the top of the dependency tree, rather than the bottom. If a "module file" is any .js file under a package.json that says type: "module", and a "commonjs file" is a .js under one says says type: "commonjs" (or is missing type: entirely), then:

• module files can import commonjs files
• commonjs files cannot require module files (and they can't use import at all)
• module files and commonjs files run under -r esm cannot import module files

So I'm going to try starting from cosmic-swingset, work my way down the dependency tree, and apply the following for each package:

• add suffixes to all imports (import foo from './bar.js' instead of import foo from 'bar')
• add type: "module" to the package.json
• remove -r esm from the test/script invocations
• bang on it until it works

[jfparadis]

Yes those ideas are the core, and they work in SES-beta/SES-shim. They also work in this POC:
https://github.com/Agoric/agoric-sdk/tree/527-modules-jf

In addition to what you listed:

1. __dirname and __filename are not available anymore. Use this workaround:

import url from 'url';
import path from 'path';

const __filename = url.fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

2. Require.resolve() is not accessible. An alternative is planned but under the --experimental-import-meta-resolve flag. It's too early to use it, IMHO, only been committed in December.
   https://github.com/nodejs/node/pull/31032/files

Instead, since we have a monorepo, we can use relative paths. This works:

  // const esend = require.resolve(`@agoric/eventual-send`);
  const esend = path.join(__dirname, '../../eventual-send/src/index.js');

3. Imports of cjs modules needs to be deconstructed:

// import { rollup as rollup0 } from 'rollup';
 import rollupCJS from 'rollup';
 const { rollup: rollup0 } = rollupCJS;

[warner]

I keep trying to approach this task, and I keep bouncing off problems. It's starting to look like I'll have to change everything all at the same time, which is a drag.

Almost all of our code is module-style (uses import and export), however Node is going to treat the file as CJS unless A: the filename is .mjs, or B: the enclosing package.json says type: "module". Given that delineation, here are the rules I understand so far:

• 1: CommonJS files cannot require() modules. (they might be able to await import(name), but that's way too hard to accomodate). This worked under -r esm, but not under Node's native module support.
• 2: modules can import CJS files
• 3: module imports of relative or deep paths must include the .js suffix
• 4: modules run under -r esm cannot import modules (I'm guessing because they're converted into CommonJS format, produce require() calls that fall under rule-1)
• 5: modules cannot do destructuring imports of CommonJS files (including module-style files which aren't marked withtype: "module")

Removing -r esm from a package (i.e. the test commands, and any executables) mandates that we switch it to type: "module", otherwise node will reject the import and export syntax. Setting a package to type: "module" mandates we remove -r esm, else Rule 4 prevents us from importing anything else (including modules from the same package). So these two changes are glued together.

Rule 4 means the parent package must stop using -r esm before the child can be marked type: "module": if the child is a module, the parent must be too, which means we can't fix this going bottom-up. A sequence that would meet Rule 4 would be to change the parent package (cosmic-swingset is at the top of the tree, as is pixel-demo) to remove -r esm and set type: "module", fix the imports (Rule 3), change the tests to run tap instead of tape. Once that all works, then we can do the same to the next child down.

But.. Rule 5 means many existing import { thing } from 'thing' lines will break if the parent is a module but the child is not. So unless we rewrite all of those (which is gross and not what we want in the long run, plus I counted 551 of them), then if a parent is a module, the child must be too, which means we can't fix this going top-down.

There are a few changes we can apply early to reduce the size of the ultimate delta:

• change import statements to add the .js prefix, which also requires a change to our eslint config: cfg.rules["import/extensions"]: ["error", { js: "ignorePackages" }] (use fully-suffixed imports (don't omit the .js) #566)
• change any .eslintrc.js files to .eslintrc.json, since the .js format is really CommonJS, and once the package is type: "module", those will be treated as modules, which won't work (use .eslintrc.json/.cjs rather than .eslintrc.js #565)
• remove any bundling steps we can
• switch the test runner from tape to tap
• replace the import { test } from 'tape-promise/tape' line with import test from 'tape-promise' (maybe? I think Rule 5 is the problem, but I think the simpler import worked for me) (import tape from 'tape', not import { tape } from 'tape' #568)

But after that, we must either rewrite a whole bunch of destructuring imports, or we have to change all our package.jsons at the same time, neither of which sounds like a lot of fun.

[michaelfig]

Thoughts, @SMotaal?

[erights]

and thoughts @bmeck @MylesBorins @guybedford ?

[warner]

Ah.. I didn't realize that Node simply does not offer named imports from CommonJS packages:

https://github.com/nodejs/node/blob/master/doc/api/esm.md#import-statements

Only the “default export” is supported for CommonJS files or packages:

import packageMain from 'commonjs-package'; // Works
import { method } from 'commonjs-package'; // Errors

That makes sense to me if I think about scanning the imported package for it's named exports (which can be done without evaluating that package). In the module-imports-module case, it can identify the named exports early, and provide an error (like the one I'm getting, "package X does not export Y") early, then do the actual module loading asynchronously. But in the module-imports-commonjs case, it doesn't have that information until evaluation time. (I was thinking this was a bug, or a problem with the way we export things, but in fact it's deeper than that).

I guess the -r esm approach was effectively taking a shortcut by evaluating the target earlier, which is different enough from the way modules are meant to work that Node doesn't do the same thing.

We've gotten in the habit of using named imports everywhere (overreliance on -r esm, I suppose). They're fine with module-imports-module, but not with module-imports-commonjs, and so far all our packages (as far as Node is concerned, because of the lack of type: "module") are commonjs. A lot of the packages we import really are commonjs, and to run under Node we're going to have to rewrite those import statements no matter what.

I prefer the clarity of named imports (and I get distracted by trying to choose a name for the temporaries), so I'd love it if our standard style was to use them. But that's just not going to be possible here, at least not when importing external packages.

I count roughly 1110 imports in packages/, of which 553 are named imports (import {). Of those, 249 are local (from '.) and another 143 are from '@agoric. 61 are import { test } from 'tape-promise/tape' which have to get rewritten no matter what. The remaining 100 have a lot of React and @material-ui imports, some import { rollup } from 'rollup', some native Node imports like child_process and path (which I assume will work just fine).

So most of those wouldn't need to be rewritten if all our repo's packages were declared as modules. To avoid the rewrite, we need to convert basically everything at the same time (which I'm trying to avoid but maybe it's the best option).

[bmeck]

Unless there is urgent reason to move to native ESM, I'd stick to compiling until Node's ESM is out of experimental status. Testing/utility frameworks have trouble with real ESM in particular if you do things like: stubbing, mocking, hot module reloading, cache invalidation, etc. There is not real path currently towards fixing that at the spec level so the only alternative is to use a runtime loader for those workflows which is back to a compilation step and Node's loader APIs are not stable.

[SMotaal]

@bmeck makes a good point, so maybe I can just elaborate on some subtle considerations.

• It is possible to work with spec compliant ESM code in src which will require things like file extensions.

  This could be lib instead of src provided they are 100% portable ESM code

• Bare specifiers can be used in Node.js and can work with import default from … but that will likely complicate browser interop:
  • I am not sure if unpkg's ?module mode will resolve bare specifiers.
  • I am not sure if there is a reliable importmap-shim out there (can't recommend).
  • I am confident ../../package-b/src/ hacks are not the way to go (don't ever please).
  • I personally use a web-first style and avoid bare specifiers altogether (not recommended otherwise).

    This looks like /packages/package-b/ and use rollup to build for node.

    • I also used the --loader route (strongly discouraged at the moment)
• Several folks have opened issues to add support for .cjs configs (prettier, eslint… etc).

My recommendation would also be to hold off moving to ESM but to also start exploring in small scale, come up with the timelines and strategy for transitioning once your particular requirements become stable. Starting ahead can help identify potential pitfalls which may be beneficial to bring to the attention in the ongoing efforts.

So, yes ESM is one thing, but migration/interop, all that is a completely different bag of tricks!

[warner]

Thanks!

Ok, I've added tickets for the cleanups we can make which work both under -r esm and type: "module". If we can get those done first, that will reduce the eventual impact of a monorepo-wide change.

I'll look more carefully at my kernel-on-new-SES branch (#477) and see if I can accomplish it without also switching away from -r esm. I was assuming that'd be a requirement, but maybe it's not, in which case we can defer the switch for a while longer.

[jfparadis]

@bmeck, unfortunately I wish it was that simple.

Testing/utility frameworks have trouble with ESM in particular if you do things like: stubbing, mocking, hot module reloading, cache invalidation, etc.

On the other hand, the same types of problems do occur with the ESM package or with any transpiration like rollup, and we found that they are much worse with them.

Testing With the ESM package, module exports are proxies, and several global object like eval(), Function() and Reflect are replaced with substitutes that introduce distortion to the expected behavior. We have found, by experience, that the issues with mocking to be either non existent for the manual mocks or the one we do with tape and tap.

On the other hand, some issues were impossible to resolve with the ESM package. Testing with the package ESM means we need to design and maintain workarounds for the way it changes the intrinsic objects.

The only workarounds we have introduced and (that Brian is discussing above) work in both modes native or transpilation. There is nothing else.

Standards This Agoric SDK library rely on the SES shim, which is to emulate the specs. Having the ESM package or any other transpiration in the system introduces more complexity and more variations.

This means bugs and uncertainty, and we can't emulate the specs because we're not close enough to the native code. Best we can do is have SES emulate what the ESM package is providing.

In other words, native ESM have us focus on import/exports while the ESM package introduces evaluators and several other objects to the mix.

Security It also means that the ESM package is part of the stack. SES and swingset are very special on that sense, they are security products and have no dependencies because we want to reduce the amount of code to vet for security.

Vetting an extra 16K lines of code just for the transpiler is just not an option for our organization.

Evolution If node wants to evolve, we need people to start using it and I believe our team is well suited to make conscious decisions about what it uses and contribute to the discussion.

@SMotaal

It is possible to work with spec compliant ESM code in src which will require things like file extensions.

We only interface with other packages in tests, and that where we need the native behavior, not some alternate one!

[SMotaal]

We only interface with other packages in tests, and that where we need the native behavior, not some alternate one!

@jfparadis So technically the portable route of ./node_modules/… is not as bad here I think, ie those tests can literally run in the browser, node… anywhere. It shouldn't offend others what anyone does in their own tests, especially if it is not included when publishing. Just a thought!

Okay I am starting to really dig the whole make…(…) approach 💯

[jfparadis]

@SMotaal, we want SES and Swingset to keep providing exports to the rest of the world via rollup.

Here is the overall approach (@warner didn't mention it, as this ticket started with a smaller subset of the discussion):

1. Remove transpilation internally

• when doing unit tests
• when importing a package inside the monorepo

In the past, we were relying on

• package rollup to provide a package export that the next package could import
• package esm for the runtime

With native ESM neither are necessary.

2. Keep/move transpilation to the periphery:

• only packages we expect to be used by others will have rollup distributions.
• this is easier to manage in a monorepo (we can clearly document which is which).
• we can have an implementation on moddable XS that is entirely native.

In the end, package esm is gone, and we only have rollup in targeted areas. On Moddable XS, neither package, rollup or esm are part of the secure toolchain, and that is super important.

Some technical detains:

We found that some workaround are necessary, but not hacks. The difference is that the workarounds are following standards, not using accidental behavior.

On the opposite, when we need to .toString() a function under the esm package, we had to do some clever editing, and I've never been comfortable with those:
https://github.com/Agoric/realms-shim/blob/master/src/utilities.js#L35

When evolving SES 2.0, we found issues with the proxies introduced by the esm package. Basically, an object defined in a module behaves differently when it is defined in a different module then imported.
https://github.com/standard-things/esm/blob/868a9029e34ba3f57498736642b879d1671f499e/src/util/proxy-exports.js

That's when we found the current approach and converted all at once. @warner is trying to do a partial convert of this monorepo, which is to change some modules to native ESM and other not. I did all at once, therefore he's finding issues I did not have to deal with.

[warner]

I was thinking, if we can get #565 and #566 landed first (which involves changing about 161 named imports of third-party packages into two-line import+destructure clauses, and maybe changing test libraries), then would the final "flag day" everything-at-once change be merely to add type: 'module' to all our package.json files at the same time? That doesn't sound so bad, it touches every package at the same time, but none of the actual code.

We got rid of all the bundling steps a few months ago. Some new tests are using tap as an assertion library, but most are using tape-promise with a named import.

It sounds like there's endo changes that must be made to accomodate this, but I think that's on a separate schedule.

[dckc]

prompted by discussion in #2988, @kriskowal @warner @michaelfig and i talked about this today. The leading proposal is:

RESM+NESM is an non-goal; rather RESM -> RESM+endo -> endo -> endo+NESM. so if a package needs multiple entrypoints, we're ok to lose NESM for that package. any RESM module will not have type: module.

@kriskowal notes to get RESM -> RESM+endo -> endo. needs endo to support import.meta.url ; and under some config, import.meta.url + __dirname
endo avoids flag day around type: module by shadowing with a different property

[michaelfig]

4: modules run under -r esm cannot import modules (I'm guessing because they're converted into CommonJS format, produce require() calls that fall under rule-1)

I found a way out of this deadlock, by patching -r esm so that it supports this style of import (#3100, which implements standard-things/esm#877). That means we can do the change into "type": "module" bottom-up at our leisure.

Removing -r esm from a package (i.e. the test commands, and any executables) mandates that we switch it to type: "module", otherwise node will reject the import and export syntax.

That's correct.

Setting a package to type: "module" mandates we remove -r esm, else Rule 4 prevents us from importing anything else (including modules from the same package). So these two changes are glued together.

This is no longer the case.

[kriskowal]

All of Agoric SDK is converted to NESM.

There is one last vestige: Dapp deployment scripts are still run with RESM.

[dckc]

Yay! Way to grind it out!

I suggest postponing ESM vs deployment scripts as a new issue and declaring victory on this one. Let me know if you'd like me to do it.

[kriskowal]

Sounds fantastic to me.

Here’s an issue for the dregs #3687