stop using esm for ES module loading

[dbushong]

BREAKING CHANGE: now if you want to use .mjs files, you need to run
with NODE_OPTIONS=--experimental-modules on Node 10.x+. Also, native
ES module loading is NOT compatible with coffeescript/register

---

This PR was started by: git wf pr

[dbushong]

Additional worthy notes:

• we've seen segfaults in the latest node 10.x during mjs loading that aren't reproducible on node 12.x.
• std/esm allowed you to "import" foo.js with import foo from './foo'; - but node native requires import foo from './foo.js';

[jkrems]

❤️

[jkrems]

nit: Would this be better as a glob-based override of the eslint settings in .eslintrc?

[dbushong]

In the next PR i'll upgrade our lint settings to ones that we can more usefully override to parse this correctly

[jkrems]

we've seen segfaults in the latest node 10.x during mjs loading that aren't reproducible on node 12.x.

Unfortunately there's no good way yet to tell if the current version of node has "real" ESM support. But I'm pretty sure that node 10 won't get backported, so maybe it would be best to restrict this to node 12+?

P.S.: Even for node 12 it's only "very likely" that we'll backport the final implementation over next few months.

[aotarola]

💚

[dbushong]

But I'm pretty sure that node 10 won't get backported, so maybe it would be best to restrict this to node 12+?

Sounds reasonable. Restrict with package.json engines.node and amending the comment to suggest Node 12+?

[jkrems]

I don't think I personally would bother with engines.node. It may be enough to call out in the README that ESM is only supported in version X of node but I would assume that if anybody is going out of their way to set --experimental-modules in node 10 and use this package, they "know" what they're doing.

[dbushong]

Turned on tests for 12.x and looks like there some bugs that need addressing if we'll be telling people to use 12. Will remove [WIP] when ready for re-review

[jkrems]

Can you try it with 13.x, potentially even without the experimental flag? There's quite a big delta between what's in 12/experimental and the code in 13.

[dbushong]

The 12.x issue is something we've seen before that's not related (AFAWCT) directly to ES Modules - it seems to be related to a change in how paths are cached inside the under-documented Module class instances. Will update as I investigate.

[dbushong]

e.g.:

$ nvm run 10 foo.js
Running node v10.17.0 (npm v6.11.3)
{ name: 'nilo-project' }
{ name: 'nilo-cli' }
$ nvm run 12 foo.js
Running node v12.13.0 (npm v6.12.0)
{ name: 'nilo-project' }
{ name: 'nilo-project' }

for foo.js:

'use strict';

const Module = require('module');

const { _nodeModulePaths: getPaths } = Module;

const m1 = new Module('<project>');
m1.filename = `${__dirname}/examples/project/app`;
m1.paths = getPaths(m1.filename);

const m2 = new Module('<cli>');
m2.filename = `${__dirname}/examples/cli/app`;
m2.paths = getPaths(m2.filename);

console.log(m1.require('./package.json'));
console.log(m2.require('./package.json'));

[dbushong]

Seems like maybe we should just move to using createRequire():

'use strict';

const Module = require('module');

const { createRequire, createRequireFromPath } = Module;

const createReq = createRequire || createRequireFromPath;

const r1 = createReq(`${__dirname}/examples/project/app`);
const r2 = createReq(`${__dirname}/examples/cli/app`);

console.log(r1('./package.json'));
console.log(r2('./package.json'));

$ nvm run 12 foo.js
Running node v12.13.0 (npm v6.12.0)
{ name: 'nilo-project' }
{ name: 'nilo-cli' }
$ nvm run 10 foo.js
Running node v10.17.0 (npm v6.11.3)
{ name: 'nilo-project' }
{ name: 'nilo-cli' }

(maybe leaving the current implementation in to make 8 happy)

[jkrems]

Oh, yeah. createRequire is a lot more reliable. Good point!

[dbushong]

Fun interesting node 10 -> 12 behavior change in Proxy:

$ nvm run 10 foo.js
Running node v10.17.0 (npm v6.11.3)
{ x: 1,
  [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] }
$ nvm run 12 foo.js
Running node v12.13.0 (npm v6.12.0)
inspected: 2

'use strict';

const INSPECT = require('util').inspect.custom;

const a = {
  x: 1,
  [INSPECT]() {
    return `inspected: ${this.x}`;
  },
};
const b = new Proxy(a, { get: () => 2 });
console.log(b);

[dbushong]

Maybe related: nodejs/node#26241

[dbushong]

Ready for re-review; thanks

[aotarola]

💚