Export mjs files so Node can natively import them. #88

dfabulich · 2020-05-02T06:28:43Z

Fixes #87.

Based on https://nodejs.org/api/esm.html it seems like there's just two things we need to do here.

The esm scripts need to have .mjs extensions
package.json needs to have an exports section, defining the ESM and CJS exports.

I think it would be nice to add built-in automated tests of this stuff, but I have no clear idea of how to do that.

Here's the test script I used to verify that I did no harm and that I fixed the bug.

#!/bin/bash -x

rm -rf testesm
mkdir testesm
cd testesm
npm init -y
npm i ../crank
cat <<EOF > index.mjs
import {createElement} from "@bikeshaving/crank";
import {renderer} from "@bikeshaving/crank/html";

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
node index.mjs

cat <<EOF > index.js
const {createElement} = require("@bikeshaving/crank");
const {renderer} = require("@bikeshaving/crank/html");

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
node index.js

brainkim · 2020-05-02T06:36:36Z

Thank you! The node people are really forcing .mjs on us huh. The one thing I’m worried about now: Will the move from .js -> .mjs mean that other tools like webpack and parcel which are extension aware break? I currently have no way to reason if this is a breaking change. I am very confused by all of it and will look for a similar package to determine what they do, because it’s very upsetting that different permutations of package.json properties and file extensions seem to produce different behaviors.

dfabulich · 2020-05-02T06:50:42Z

Good thinking on parcel. I'd totally broken the parcel build when I tried this test

#!/bin/bash

rm -rf testparcel
mkdir testparcel
cd testparcel
npm init -y
npm i parcel-bundler ../crank
cat <<EOF > index.html
<html>
<body>
<script src="index.js"></script>
</body>
</html>
EOF

cat <<EOF > index.js
/** @jsx createElement */
import {createElement} from "@bikeshaving/crank";
import {renderer} from "@bikeshaving/crank/dom";

renderer.render(<div>Hello world</div>, document.body);
EOF

cat <<EOF > .babelrc
{"presets":["@babel/preset-react"]}
EOF

npx parcel index.html --open

I'd forgotten to update dom.js and html.js in the root directory. After I updated those, this test passed.

dfabulich · 2020-05-02T06:54:01Z

Per the is-promise post mortem, I believe this is a "breaking" change. For example, require('@bikeshaving/crank/cjs/index.js') used to work, but would no longer work after merging this PR (due to the exports changes.

dfabulich · 2020-05-02T07:05:27Z

… and that's somewhat regrettable because this PR will break the workaround for issue #89. On the bright side, it also fixes issue #89…but only on node 13.2 and up. (Node 13.1 and lower don't honor exports in package.json

rm -rf wtfcjs
mkdir wtfcjs
cd wtfcjs
npm init -y
npm i ../crank
cat <<EOF > index.js
const {createElement} = require("@bikeshaving/crank");
const {renderer} = require("@bikeshaving/crank/html");

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
node index.js

dfabulich · 2020-05-02T07:12:52Z

I believe I fixed breaking the workaround for #89 in the most recent push.

ryhinchey · 2020-05-02T10:41:40Z

We should be able to test that this works correctly in our CI tests at least. I’ll provide some more thoughts on this later today

brainkim · 2020-05-02T17:25:21Z

it also fixes issue #89…but only on node 13.2 and up. (Node 13.1 and lower don't honor exports in package.json

😱😱😱😱😱😱
It‘s when I read things like this that I want to migrate everything to deno.

nykula · 2020-05-03T13:35:53Z

Out of curiosity, node 13 is an intentionally unstable major and will be officially dead in a month, why put effort into targeting its already unsupported older .1 minor? As much as I liked commonjs, it didn't make into the standard and won't ever become one now, there will be fewer and fewer of such npm modules from now on. For maintenance conceptual simplicity, I'd keep only the standard esm code in the main crank package, with extension .js and type:module in package.json, like in one of the options the mentioned node doc suggests. And post a separate es5 crankcjs package for everyone irreparably stuck with a legacy runtime, right away with a deprecation warning.

dfabulich · 2020-05-03T16:39:08Z

I’m not going to any special effort. I just noticed that Node 12 users will continue to suffer from #89 after this PR merges, but Node 14 users won’t. #89 has a workaround, which this PR temporarily broke, but I fixed that in my PR. If you’re working around #89 in Node 12 or 14, it will continue to work after this PR.

ryhinchey · 2020-05-03T19:07:53Z

Not that it needs to be done as part of this PR but I think there are two ways we could test this:

we could use the if syntax in github action workflows to run different tests based on the node environment.
a custom script that checks process.version against the semver package.

We could have scripts that runs in 12, 13, and 14 ensuring Crank loads correctly in each version.

This all might be more work than it's worth though

dfabulich · 2020-05-03T20:16:42Z

The tests I would run are:

A test to ensure that the package.json exports section points to real files
An ESM test on Node 14 (or latest LTS) to natively import all of the import subpaths in exports, just ensuring that they can be imported from a .mjs file without throwing. Currently that means just importing @bikeshaving/crank, @bikeshaving/crank/dom, and @bikeshaving/crank/html.
A CJS test on Node 14 (or latest LTS) to require @bikeshaving/crank, @bikeshaving/crank/dom, and @bikeshaving/crank/html.

brainkim · 2020-05-03T20:23:58Z

So just to be clear there is no way for us to not use .mjs files? "type": "module" doesn’t work for arcane reasons? I’m trying to investigate this and I thought forcing cjs users to use the cjs directory would have been a reasonable compromise. I don’t think anyone seems to know what is going on.

dfabulich · 2020-05-03T20:31:53Z

It's a little bold for me to claim that there's "no way" to do something, but I just pushed this branch https://github.com/dfabulich/crank/tree/buggy with this commit dfabulich@cbe135d that switches back from .mjs to .js. When I ran the testesm script described in the PR description, I get this error:

$ node index.mjs
file:///private/tmp/testesm/index.mjs:2
import {renderer} from "@bikeshaving/crank/html";
        ^^^^^^^^
SyntaxError: The requested module '@bikeshaving/crank/html' does not provide an export named 'renderer'
    at ModuleJob._instantiate (internal/modules/esm/module_job.js:92:21)
    at async ModuleJob.run (internal/modules/esm/module_job.js:107:20)
    at async Loader.import (internal/modules/esm/loader.js:179:24)

dfabulich · 2020-05-04T07:45:47Z

I did some research on why my https://github.com/dfabulich/crank/tree/buggy branch is buggy, and I turned up a bug in Node!

The core idea of this PR is to use the new-ish Node conditional exports system, associating paths with files depending on how they're requested (import or require). Here's the key bit:

  "exports": {
    ".": {
      "import": "./esm/index.js",
      "require": "./cjs/index.js"
    },
    "./html": {
      "import": "./esm/html.js",
      "require": "./cjs/html.js"
    },
    "./dom": {
      "import": "./esm/dom.js",
      "require": "./cjs/dom.js"
    },

Thus, if you import '@bikeshaving/crank/html', you should get ./esm/html.js, and if you require('@bikeshaving/crank/html'), you should get ./cjs/html.js. So far, so good.

The problem is that conditional exports don't require that the import script be an ESM script. The doc explains:

"import" - matched when the package is loaded via import or import(). Can reference either an ES module or CommonJS file, as both import and import() can load either ES module or CommonJS sources.

I guess it's "nice" that "import" is so flexible, but it means that Node has no idea that ./esm/html.js is an ESM script. Instead, Node just follows its standard rules, which treat everything as CJS, except:

Files ending in .mjs.

Files ending in .js when the nearest parent package.json file contains a top-level field "type" with a value of "module".

So, in practice, the testesm script is importing ./esm/html.js in CJS mode.

As a result of this confusion, you might expect that html.js would throw a SyntaxError at its first call to import. But, due to a bug in Node, it doesn't even get that far, because Node notices that ./esm/html.js is CJS, and that it can therefore provide no named exports. Node doesn't even try to parse ./esm/html.js in CJS mode; it just quits fast with this misleading error:

The requested module '@bikeshaving/crank/html' does not provide an export named 'renderer'

This was filed a couple of months ago, as nodejs/node#32137 "Misleading error that module does not provide export"

brainkim · 2020-05-04T08:10:26Z

Files ending in .js when the nearest parent package.json file contains a top-level field "type" with a value of "module".

But what I’m asking is, if we add "type": "module", would that not fix the .mjs stuff too?

dfabulich · 2020-05-04T08:28:29Z

I feel faintly silly suggesting this, but I've got another branch https://github.com/dfabulich/crank/pull/new/explicit-cjs that works surprisingly well. At least, better than I'd expected.

In that branch, instead of using .mjs for ESM scripts (and all the weirdness that entails), we set "type": "module" in package.json, which makes all of the scripts ESM by default, except for scripts with an explicit .cjs suffix.

This all works great in Node 14, both in CJS and ESM mode, thanks to the "exports" map which routes everybody to the right places. Even Parcel and Rollup pretty much just do the right thing, both with CJS and ESM scripts.

Unfortunately, the explicit-cjs branch does break backwards compatibility with anyone on Node 12 who used the documented workaround for #89 to require('@bikeshaving/crank/cjs'), because there's no longer a .js file in that directory. In the explicit-cjs branch, Node 12 users would have to workaround #89 like this:

const {createElement} = require("@bikeshaving/crank/cjs/index.cjs");
const {renderer} = require("@bikeshaving/crank/cjs/html.cjs");

That workaround would continue to work in Node 14, though it would be unnecessary.

This might be a better approach just because Parcel, Rollup, and Webpack seem to get a little weird around .mjs files, and none of them yet support "exports" mappings out of the box.

parcel-bundler/parcel#4155 "Use package.json#exports map"
https://github.com/lukeed/webpack-modules "Handle .mjs files correctly. Because webpack does it wrong and won't fix it."

It does feel faintly more correct to automatically define ./html.js and ./dom.js as ESM scripts (which they are) without having to rename them.

brainkim · 2020-05-05T20:11:50Z

I’m continuing to investigate this, but I can only do it in limited amounts because the state of esmodules support in node.js makes me so upset. The community told node maintainers over and over they wanted absolutely nothing to do with .mjs files and they kept pushing em on us, adding successive amounts of configuration across different versions of node which are poorly explained, error in cryptic ways for consumers, and depart from conventions established by package.json itself. It’s such a terrible situation and right now I’m thinking that I will refuse to support or output .mjs/.cjs files out of principle. I’m really fuming right now and I want to say lots of mean things. I can’t believe things could get this fucked up. What an absolute abdication of leadership.

Sorry. Thanks for your help @dfabulich, but understand when I say that we as developers need to take a stand when it comes to complexity, especially complexity with regards to modules and configuration, and I can’t just merge this in without a lot of convincing even if things might be “broken.”

dfabulich · 2020-05-05T22:48:01Z

Lemme try to calm you down a little. :-)

MJS and CJS don't interop well

The main thing that sucks is that ESM scripts can't natively import named exports from CJS, i.e. import { blah } from './dom.cjs' fails. The default export is fine, i.e. import dom from './dom.cjs' works; just the named exports are broken.

I don't fully understand why this is, but I believe there's some subtle runtime interop issue around CommonJS caching(?), and that the Node modules team tried hard to make it work; they only reluctantly gave up.

They updated the docs in November where previously they said:

Currently only the “default export” is supported for CommonJS files. [...] There are ongoing efforts to make the latter code possible.

But they removed the "Currently," and the line saying that "there are ongoing efforts." :-(

The interop issue is nobody's fault

But why did this happen? At the end of the day, ESM was primarily designed for browsers, and designed together by the browser vendors (Google, Apple, Microsoft, and Mozilla) to minimize network overhead. The browser vendors gave only a casual thought to Node interop in this process.

What you call "an absolute abdication of leadership," I'd call "prioritizing browser performance over Node interop." Browser performance is a really important problem! If Node has to undergo a painful transition period to make import work natively in the browser with good performance, then that may have been the best decision from a global "leadership" perspective.

I certainly can't blame the Node modules team for not convincing three multi-billion-dollar corporations to tweak the design on their behalf, and neither do I blame the browser vendors for prioritizing browser performance over Node interop.

The fact that MJS and CJS don't play well together is nobody's fault. Everybody was trying hard to do the right thing. But everything that sucks about this problem rolls downhill from that issue.

Conditional exports are good, actually

If you start with the understanding that separating ESM from CJS is necessary, then I actually think the conditional exports exports map in package.json is a really good solution; I wish the community had cooked up something like it years ago. Sadly, I think none of today's major bundlers honor it, but it'll be a pretty good thing when they do, and there are ways to preserve backwards compatibility with them (which this PR does).

Now, it does suck that there's no way to say to Node "these .js files are ESM; these .js files are CJS." Superficially, it seems like exports would do that, but it doesn't. It says, "this is what ESM should import," but since ESM can import CJS, saying "ESM should import dom.js" doesn't entail "dom.js is ESM."

The only way to mix-n-match CJS and MJS today is via file extensions, either .mjs extensions, or setting "type": "module" and using .cjs extensions. I think it would be nicer if there were some setting in package.json allowing us to say "everything in the cjs directory is CJS; everything in the mjs directory is MJS."

However! The exports map allows us to encapsulate the file extension from library consumers. ESM users never need to know or care that the output files have .mjs extensions.

It still sucks, though

I could imagine someone saying that since the exports map encapsulates the file extensions, the actual file names are no big deal.

But no, you're right about at least one thing, .mjs is a big deal, because .mjs files have these weird runtime interop problems. Unfortunately, that would be true even if package.json provided a way to declare which files/directories are CJS/MJS.

Still, the problem isn't the file extension. The problem is that the ESM scripts and the CJS scripts don't get along; .mjs is just the "Abandon hope" sign over the gates of hell.

So let's merge this thing

If you agree with everything I've written here, I'd ask you to merge this PR and move on with our lives.

If you want to "take a stand," at least identify some specific proposal issue in the https://github.com/nodejs/modules/issues repository to fight for. Say that fixing #87 is "blocked" on https://github.com/nodejs/modules/issues/99999 or whatever, and work through the problems in that issue. But if you don't have a specific proposal to fight for, if you're just waiting around hoping that someone else will lead, let's merge this thing while we wait.

dfabulich · 2020-05-05T23:06:06Z

I looked it up. nodejs/modules#81

The reason that NodeJS doesn’t allow named exports is that determining what those exports are MUST happen in the parsing phase of the ESM Spec (according to some, although there is contention about that too), and executing code is not allowed at or prior to that phase. But a CJS module’s exports can only be determined by evaluating the code in it

Some folks suggested just executing CJS imports before ESM imports, out of order, but:

nodejs/modules#81 (comment)

Even if we disregard the specification talk for a bit; the problem with out of order execution is that you could no longer order/refactor your imports if they have side effects:
import 'a';
import 'b';
Changing b from ESM to CJS would change the ordering from a, b to b, a. Which would be problematic if b relied on something from a being executed first.

Fixes bikeshaving#87.

dfabulich · 2020-05-15T18:04:04Z

Any thoughts since last week?

brainkim · 2020-05-15T18:21:09Z

@dfabulich I apologize for leaving this open for so long! I’ll make sure this or another solution gets pushed out by Sunday at latest. I’m currently exploring performance fixes, and as it turns out, using untranspiled generator objects is much much better than transpilation so I want to batch all these changes for the first breaking change release (0.2.0), where we ship crank uncompiled for esmodules, as well as shipping UMD builds (#45) and maybe mangling some methods so the resulting bundle output is smaller.

As far as the configuration maze goes. I prefer:

no mjs or cjs files outputted
only cjs files outputted
both mjs files and cjs files outputted

in that order. I know, I know, I just want to see if it’s actually possible because I think it’s increasingly possible that node dies in the next half-decade and I don’t want to be stuck supporting these terrible file extensions. Of course, I will defer to your expertise on this matter and if you say something is not possible I will believe you (after trying for a couple hours to do it myself).

If this means that people on CommonJS environments have to go out of their way to adjust paths to files or upgrade, I’m okay with that. ESModule support should be first-class.

dfabulich · 2020-05-15T18:53:16Z

I claim that your option 1 (.js only) is not workable; Node will either interpret all .js files as CJS or all of them as ESM, depending on whether package.json says "type": "module".

Option 2 (.js ESM .cjs CJS) is a little worse for backwards compatibility, because the docs currently recommend that CJS users require the cjs directory like this require('@bikeshaving/crank/cjs'). That will work in Node 14 but won't work in Node 12, because it will look for cjs/index.js and won't look for cjs/index.cjs. But we can definitely do Option 2, and I have an example in my explicit-cjs branch. https://github.com/dfabulich/crank/pull/new/explicit-cjs

To be clear, the current draft of the PR does a fourth thing, Option X, .js for CJS and .mjs for ESM. I claim that Option X is at least better than your Option 3, and I thought it would be better than Option 2 (because it would preserve backward compatibility better).

But if your sense is that backwards compatibility is unimportant (because Crank isn't 1.0, and perhaps because deno ESM is the future?) then I can reroll this PR with the explicit-cjs branch and move ahead that way; that would be fine by me.

brainkim · 2020-05-15T19:40:49Z

Yeah, I care less about breaking changes with regard to module formats and bundling, because those are easier upgrades than actual changes to framework logic, and it’s important to get this right before 1.0. And because esmodules are clearly the future, I think it’s better to have the non-standard file format be the cjs files.

I claim that your option 1 (.js only) is not workable; Node will either interpret all .js files as CJS or all of them as ESM, depending on whether package.json says "type": "module".

One possible solution I thought while in the shower the other day that I have yet to try: copying the package.json into the cjs directory and changing the type. Whether that’s a good idea or not, who knows 😉 If you end up figuring out all of this stuff I would very much like to read a blog post or gist about it. If I write it all up it would probably end up too sweary and toxic.

dfabulich · 2020-05-16T05:14:56Z

OMG that worked! But… I'm not exactly certain how to put this in a PR yet.

https://github.com/dfabulich/crank/pull/new/subpackage-json

That PR changes nothing about the build, but only changes package.json. At the root package.json, we add exports and set "type":"module". The branch also creates a cjs/package.json that sets "type":"commonjs".

The problem is, when you npm run build in that branch, it deletes cjs/package.json, so I currently have to manually git checkout cjs/package.json to restore the file.

What's the best way to install cjs/package.json during the build? Do we have to write a rollup plugin for this??

Anyway, once I npm run build && git checkout cjs/package.json, I'm able to run this test script on Node 14:

test script

#!/bin/bash -x

rm -rf testesm
mkdir testesm
cd testesm
npm init -y
npm i ../crank
cat <<EOF > index.mjs
#!/usr/bin/env node --experimental-modules
import {createElement} from "@bikeshaving/crank";
import {renderer} from "@bikeshaving/crank/html";

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
chmod +x index.mjs
./index.mjs

cat <<EOF > workaround.js
#!/usr/bin/env node
const {createElement} = require("@bikeshaving/crank/cjs");
const {renderer} = require("@bikeshaving/crank/cjs/html");

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
chmod +x workaround.js
./workaround.js

cat <<EOF > index.js
#!/usr/bin/env node
const {createElement} = require("@bikeshaving/crank");
const {renderer} = require("@bikeshaving/crank/html");

console.log(renderer.render(createElement("div", null, "Hello world")));
EOF
chmod +x index.js
./index.js

The script mostly works on Node 12, except that require("@bikeshaving/crank/html") still doesn't work on Node 12 (issue #89), because Node 12 doesn't support exports. (As far as I'm aware, there's no way to fix #89 in Node 12; we can only fix #89 in versions of Node that support exports, i.e. 13.2 and up.)

Since that's not a regression, I'd say this is a big step forward!

brainkim · 2020-05-16T18:42:39Z

@dfabulich There is always a way 😄 I guess the best way to do it would be a rollup plugin, but I also wonder if we can’t just use fs.writeFileSync in rollup.config.js. I’m also wondering if we have to support the exports stuff if we use this solution? I’m looking at the exports stuff and it feels like a footgun in the sense that we have to make sure that files and exports always match in case we want to expose other files.

brainkim · 2020-05-17T00:27:51Z

@dfabulich Does #108 solve every permutation of file extension, node version, command-line flag and build system that we’re interested in? I have to go smoke a cigarette because I’m angry again at the state of things.

dfabulich · 2020-05-17T06:31:01Z

This PR is dead; we're going with #108 instead.

dfabulich force-pushed the dgf-node-esm branch from bb7b61a to 420c24e Compare May 2, 2020 06:31

dfabulich force-pushed the dgf-node-esm branch from 420c24e to 4d30bb3 Compare May 2, 2020 06:44

dfabulich force-pushed the dgf-node-esm branch from 4d30bb3 to ad95eca Compare May 2, 2020 07:10

dfabulich force-pushed the dgf-node-esm branch from ad95eca to 7e1f11f Compare May 2, 2020 08:10

brainkim self-requested a review May 2, 2020 19:45

dfabulich mentioned this pull request May 7, 2020

Crank not installing correctly on Snowpack 2 #99

Closed

Export mjs files so Node can natively import them.

48023bb

Fixes bikeshaving#87.

dfabulich force-pushed the dgf-node-esm branch from 7e1f11f to 48023bb Compare May 15, 2020 18:03

dfabulich closed this May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export mjs files so Node can natively import them. #88

Export mjs files so Node can natively import them. #88

dfabulich commented May 2, 2020

brainkim commented May 2, 2020

dfabulich commented May 2, 2020

dfabulich commented May 2, 2020

dfabulich commented May 2, 2020 •

edited

dfabulich commented May 2, 2020

ryhinchey commented May 2, 2020

brainkim commented May 2, 2020

nykula commented May 3, 2020

dfabulich commented May 3, 2020

ryhinchey commented May 3, 2020

dfabulich commented May 3, 2020

brainkim commented May 3, 2020

dfabulich commented May 3, 2020

dfabulich commented May 4, 2020

brainkim commented May 4, 2020

dfabulich commented May 4, 2020 •

edited

brainkim commented May 5, 2020 •

edited

dfabulich commented May 5, 2020

dfabulich commented May 5, 2020

dfabulich commented May 15, 2020

brainkim commented May 15, 2020 •

edited

dfabulich commented May 15, 2020 •

edited

brainkim commented May 15, 2020 •

edited

dfabulich commented May 16, 2020

brainkim commented May 16, 2020

brainkim commented May 17, 2020

dfabulich commented May 17, 2020

Export mjs files so Node can natively import them. #88

Export mjs files so Node can natively import them. #88

Conversation

dfabulich commented May 2, 2020

brainkim commented May 2, 2020

dfabulich commented May 2, 2020

dfabulich commented May 2, 2020

dfabulich commented May 2, 2020 • edited

dfabulich commented May 2, 2020

ryhinchey commented May 2, 2020

brainkim commented May 2, 2020

nykula commented May 3, 2020

dfabulich commented May 3, 2020

ryhinchey commented May 3, 2020

dfabulich commented May 3, 2020

brainkim commented May 3, 2020

dfabulich commented May 3, 2020

dfabulich commented May 4, 2020

brainkim commented May 4, 2020

dfabulich commented May 4, 2020 • edited

brainkim commented May 5, 2020 • edited

dfabulich commented May 5, 2020

MJS and CJS don't interop well

The interop issue is nobody's fault

Conditional exports are good, actually

It still sucks, though

So let's merge this thing

dfabulich commented May 5, 2020

dfabulich commented May 15, 2020

brainkim commented May 15, 2020 • edited

dfabulich commented May 15, 2020 • edited

brainkim commented May 15, 2020 • edited

dfabulich commented May 16, 2020

brainkim commented May 16, 2020

brainkim commented May 17, 2020

dfabulich commented May 17, 2020

dfabulich commented May 2, 2020 •

edited

dfabulich commented May 4, 2020 •

edited

brainkim commented May 5, 2020 •

edited

brainkim commented May 15, 2020 •

edited

dfabulich commented May 15, 2020 •

edited

brainkim commented May 15, 2020 •

edited