Skip to content

Commit

Permalink
Fix mjs structure to work with node v12
Browse files Browse the repository at this point in the history
  • Loading branch information
@mike-marcacci
mike-marcacci committed May 5, 2019
1 parent 34eed84 commit c606d33
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 14 deletions.
5 changes: 5 additions & 0 deletions changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,8 @@
- Updated dependencies.
- Move configs out of package.json
- Use `wx` file flag instead of default `w` (thanks to @mattbretl via #8)

### 2.0.2

- Updated dev dependencies.
- Fix mjs structure to work with node v12.
20 changes: 7 additions & 13 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
"lib",
"!lib/test.*"
],
"main": "lib",
"module": "lib/index.mjs",
"main": "lib/index.js",

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 5, 2019

Collaborator

Pretty sure this will break native ESM, as you are directing all versions and modes of Node.js to enter the CJS .js file. Note that Node.js does not use the package module field; only bundlers like Webpack and Rollup use that.

This comment has been minimized.

Sign in to view

Copy link
@mike-marcacci

mike-marcacci May 5, 2019

Author Owner

Hey @jaydenseric thanks so much for looking over this – the challenge is that node 12 totally changed the way modules are resolved: https://github.com/nodejs/modules/blob/master/doc/announcement.md

Unless I'm missing something, it's no longer possible to use a pair of foo.js and foo.mjs files, since node now requires complete paths for any imports using es modules.

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 5, 2019 •
edited
Loading

Collaborator

I've been following the nodejs/modules work closely. They regressed in Node.js v12 by removing a few features from --experimental-modules mode, which are breaking changes. The new feature set is not yet complete, and in v12 there is no way to do dual mode ESM/CJS packages anymore. There is a reasonable chance that the old behavior will be restored in time for an unflagged release. For now, I am simply not supporting Node.js v12 --experimental-modules and will only update my packages once the permanent, complete implementation is ready to be unflagged.
"module": "src/index.mjs",

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 5, 2019 •
edited
Loading

Collaborator

You don't want to point at the src/, as that is not transpiled! Webpack and Rollup users will be bundling untranspiled code. CJS and ESM published code should be identical except for the module transforms. If you are absolutely sure that @babel/preset-env is doing nothing except for that, it's best to remove that preset and use the module transforms on their own.

This comment has been minimized.

Sign in to view

Copy link
@mike-marcacci

mike-marcacci May 5, 2019

Author Owner

I found that in the case of this module, the only transformations happening were the removal of comments, which did nothing useful and changed line numbers in stack traces. I agree I should remove that preset.
"engines": {
"node": ">=8.5"
},
Expand All @@ -36,7 +36,7 @@
"eslint-config-prettier": "^4.0.0",
"eslint-plugin-import": "^2.16.0",
"eslint-plugin-import-order-alphabetical": "^0.0.2",
"eslint-plugin-node": "^8.0.1",
"eslint-plugin-node": "^9.0.1",
"eslint-plugin-prettier": "^3.0.0",
"husky": "^2.2.0",
"leaked-handles": "^5.2.0",
Expand All @@ -45,17 +45,11 @@
"tap": "^13.1.2"
},
"scripts": {
"prepare": "npm run prepare:clean && npm run prepare:mjs && npm run prepare:js && npm run prepare:prettier",
"prepare:clean": "rm -rf lib",
"prepare:mjs": "BABEL_ESM=1 babel src -d lib --keep-file-extension",
"prepare:js": "babel src -d lib",
"prepare:prettier": "prettier 'lib/**/*.{mjs,js}' --write",
"test": "npm run test:eslint && npm run test:prettier && npm run test:mjs && npm run test:js",
"prepare": "rm -rf lib && babel src/index.mjs -d lib && prettier 'lib/**/*.js' --write",
"test": "npm run test:eslint && npm run test:prettier && npm run test:tap",
"test:eslint": "eslint . --ext mjs,js",
"test:prettier": "prettier '**/*.{json,yml,md}' -l",
"test:mjs": "node --experimental-modules --no-warnings lib/test | tap-mocha-reporter spec",
"test:js": "node lib/test | tap-mocha-reporter spec",
"prepublishOnly": "npm run prepare && npm test",
"watch": "watch 'npm run prepublishOnly --silent' src --interval 1"
"test:tap": "node --experimental-modules --no-warnings src/test | tap-mocha-reporter spec",
"prepublishOnly": "npm run prepare && npm test"
}
}
2 changes: 1 addition & 1 deletion src/test.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import "leaked-handles";
import fs from "fs";
import stream from "stream";
import t from "tap";
import WriteStream, { ReadAfterDestroyedError } from ".";
import WriteStream, { ReadAfterDestroyedError } from "./index.mjs";

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 5, 2019

Collaborator

By putting the full path including file extension in the import, you are not accurately testing how consumers normally import:

import WriteStream from 'fs-capacitor' // <- No full path with file extension.

This comment has been minimized.

Sign in to view

Copy link
@mike-marcacci

mike-marcacci May 5, 2019

Author Owner

Aaah, that is a fantastic point; and is precisely the workaround I needed. For libs of any greater complexity than this, though, the changes in node 12 will make it quite difficult to export both cjs and mjs.

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 5, 2019 •
edited
Loading

Collaborator

If the latest version of fs-capacitor is working in Node.js v10 and v12 --experimental-modules (I haven't checked properly yet in my projects), it might be because most people only consume the default import, so loading the CJS will have the same effect as not shipping ESM at all.

This comment has been minimized.

Sign in to view

Copy link
@mike-marcacci

mike-marcacci May 5, 2019

Author Owner

Wow ya, this is quite frustrating... I mean, I guess it was under an --experimental- flag, but I figured that a breaking change would be a change in how one ships cjs and mjs... not a complete removal of the ability. I'm using this package in some migrations that are likely going to run on node 12. They don't need to use native modules, but I definitely want to continue supporting it.

I think these are the priorities, in order of first importance:

  1. support commonjs in node 8 - 12
  2. support es modules in bundlers in node 8 - 12
  3. support native es modules in node 8 - 11
  4. support native es modules in node 12

I think you're right that the current implementation only works with 1 and 2, so I'll get it fixed ASAP to support 3 again. Let me know if you find a way to support 4. Is there a mailing list or issue I should subscribe to for this?

This comment has been minimized.

Sign in to view

Copy link
@jaydenseric

jaydenseric May 6, 2019

Collaborator

They are debating several options for dual mode:

  • Exports main #41
  • Proposal for dual ESM/CommonJS packages #273
  • Proposal for single-mode packages with optional fallbacks for older versions of node #299

const streamToString = stream =>
new Promise((resolve, reject) => {
Expand Down

0 comments on commit c606d33

Please sign in to comment.