type: module breaks users on various version of npm

[jdalton]

Just a heads up I bumped lodash-es to use type of module and got reports of breakage due to various npm versions. See https://github.com/lodash/lodash/issues/4359.

[devsnek]

the linked issue's trace isnt entitely clear to me. does npm already use the type field for something?

[hybrist]

Thanks for raising this! The type field causing issues with existing tools sounds like a strong reason to reconsider the name. But to echo @devsnek I think we need to understand where exactly npm is trying to use it, especially during install. That seems super weird.

Is there anybody from the npm team that would be the go-to to ping in this context? It looks like the most recent CLI commits are by @isaacs himself but I'm not sure if he'd have context on the affected versions of the CLI.

[ljharb]

npm mutates package.json in installed packages by adding additional metadata (like requiredBy etc); perhaps some versions added “type” when not present and then attempted to read it back later in the install?

[hybrist]

From skimming the code, my guess:

1. The npm CLI merges the package data into an object literal to express a combined "install request".
2. It uses { type: 'directory' } for things installed from a directory source.
3. If type is present but not set to directory, it blows up.

https://github.com/npm/cli/blob/c1522be2406a0ea4f14c85753edd42ddd8d7e180/lib/fetch-package-metadata.js#L67

[hybrist]

So yeah, without fixing npm (this is present on latest master) and waiting for all users to cycle out the affected versions, type is a non-starter.

[robpalme]

Is it mandatory for Node to retain compatibility with every version of npm there ever was? Is there a version range we can reduce it down to?

On Wed, 10 Jul 2019, 17:47 Jan Olaf Krems, [email protected] wrote:

So yeah, without fixing npm (this is present on latest master) and waiting for all users to cycle out the affected versions, type is a non-starter.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/modules/issues/348?email_source=notifications&email_token=ABU6F5OQGNDMRCIACGTN25LP6YABNA5CNFSM4H7PSUO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZT4TIQ#issuecomment-510118306, or mute the thread https://github.com/notifications/unsubscribe-auth/ABU6F5IDVIBXTCJCGT5L3GTP6YABNANCNFSM4H7PSUOQ .

[guybedford]

This is odd as I've had "type" in use in the SystemJS repo for some time and haven't had any issues at all there... perhaps it is only for packages with dependencies? It would be good to properly confirm the exact replication and versions before jumping to conclusions (eg @jkrems what makes you so sure it affects latest master? Users indicated npm install lastest fixed it).

[GeoffreyBooth]

Another option is to move type one level down underneath a namespace, like nodejs. Especially as we're considering adding more fields (exports, etc.) this would protect us from further conflicts.

[ljharb]

It doesn’t matter if it’s mandatory or not; node supporting a convention that forces packages to break compat with older npm users seems catastrophic.

It’s likely that lodash is used by people on older nodes/npms more than systemJS is.

It would be good to better understand the conditions here, but i think the top-level name “type” is immediately not an option based on this.

[hybrist]

@jkrems what makes you so sure it affects latest master? Users indicated npm install lastest fixed it

I may have jumped to a conclusion from seeing suspicious code in latest master. Maybe that code is no longer affected because there's protection somewhere else.

Is it mandatory for Node to retain compatibility with every version of npm there ever was? Is there a version range we can reduce it down to?

The type field may be added by packages that can run outside of node (e.g. universal code). If adding the field makes them incompatible with being installed on older versions of node, their only reasonable reaction is to back it out. Because they may have perfectly reasonable consumers that build a SPA using node 8 or 10 and just want their build to not break.

[jdalton]

For more context from the lodash-es issue it looks like it could be older versions of npm (see issue and linked related issues for specifics). Also, I'm totally not suggesting a rename of the field, I'm just following "see something, say something" is all 🙃

Update:

Feel free to pop-in to the lodash issue and ask folks for more details.

[GeoffreyBooth]

I can reproduce the issue:

✦ docker run node:6.7.0 bash -c 'npm install [email protected]'
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm info attempt registry request try #1 at 11:44:33 PM
npm http request GET https://registry.npmjs.org/lodash-es
npm http 200 https://registry.npmjs.org/lodash-es
npm ERR! Linux 4.9.125-linuxkit
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "i" "[email protected]"
npm ERR! node v6.7.0
npm ERR! npm  v3.10.3
npm ERR! code EMISSINGARG

npm ERR! typeerror Error: Missing required argument #1
npm ERR! typeerror     at andLogAndFinish (/usr/local/lib/node_modules/npm/lib/fetch-package-metadata.js:31:3)
npm ERR! typeerror     at fetchPackageMetadata (/usr/local/lib/node_modules/npm/lib/fetch-package-metadata.js:51:22)
npm ERR! typeerror     at resolveWithNewModule (/usr/local/lib/node_modules/npm/lib/install/deps.js:515:12)
npm ERR! typeerror     at /usr/local/lib/node_modules/npm/lib/install/deps.js:243:5
npm ERR! typeerror     at /usr/local/lib/node_modules/npm/node_modules/slide/lib/async-map.js:52:35
npm ERR! typeerror     at Array.forEach (native)
npm ERR! typeerror     at /usr/local/lib/node_modules/npm/node_modules/slide/lib/async-map.js:52:11
npm ERR! typeerror     at Array.forEach (native)
npm ERR! typeerror     at asyncMap (/usr/local/lib/node_modules/npm/node_modules/slide/lib/async-map.js:51:8)
npm ERR! typeerror     at exports.loadRequestedDeps (/usr/local/lib/node_modules/npm/lib/install/deps.js:241:3)
npm ERR! typeerror This is an error with npm itself. Please report this error at:
npm ERR! typeerror     <http://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /npm-debug.log

Note that that’s with Node 6.7.0, the version of Node reported in the original issue, and the version of npm that shipped with that version of Node (3.10.3, printed above). Correct me if I’m wrong but I think Node 6 has already been retired, and currently the oldest supported version of Node is 8.0.0. The problem is not present in Node 8.0.0 and the version of npm that shipped with it:

✦ docker run node:8.0.0 bash -c 'npm install [email protected]'
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm http fetch GET 200 https://registry.npmjs.org/lodash-es 6976ms
npm http fetch GET 200 https://registry.npmjs.org/lodash-es/-/lodash-es-4.17.13.tgz 4046ms
npm info lifecycle [email protected]~preinstall: [email protected]
npm info linkStuff [email protected]
npm info lifecycle [email protected]~install: [email protected]
npm info lifecycle [email protected]~postinstall: [email protected]
npm WARN saveError ENOENT: no such file or directory, open '/package.json'
npm info lifecycle undefined~preshrinkwrap: undefined
npm info lifecycle undefined~shrinkwrap: undefined
npm notice created a lockfile as package-lock.json. You should commit this file.
npm info lifecycle undefined~postshrinkwrap: undefined
npm WARN enoent ENOENT: no such file or directory, open '/package.json'
npm WARN !invalid#1 No description
npm WARN !invalid#1 No repository field.
npm WARN !invalid#1 No README data
npm WARN !invalid#1 No license field.

added 1 package in 11.482s
npm info ok

So basically, the version of npm (5.0.0) that shipped with the oldest still-supported version of Node (8.0.0) does not have this issue. We could simply tell users that sorry, but the time has come for them to upgrade at least their version of npm; or to set up a mirror of the npm registry and use it to control the versions of packages that they allow their build pipeline to install; or to commit their node_modules folder into their repo, since this probably isn’t an actively developed app anymore and probably shouldn’t be automatically receiving all the latest versions of every dependency. I’m not sure how reasonable it is to ask package authors to refrain from adopting new features to support unsupported versions of Node and npm, especially when there are workarounds for users that cannot upgrade.

I also tested with the newest version of Node 6 (6.17.1 / npm 3.10.10, via docker run node:6 bash -c 'npm init --yes; npm install [email protected]' and it works there too. So even if you cannot upgrade to Node 8, if you can at least upgrade to the newest Node 6 (or npm 3.10.10) you won’t have this issue.

[hybrist]

That sounds pretty promising! Thanks for digging into it, @GeoffreyBooth. But I think one important question is - would maintainers of a popular package add it and keep it even when stray node 6 users (with default npm) complain? Or maybe: When would it be realistic that they would keep it?

@jdalton Is this something you'd still move forward with, e.g. in lodash-es@5? Or is it too early to tell for you?

[MylesBorins]

I'm personally fine with this breakage. It is in an unsupported version of node and there is a quick way to fix , update npm

On Thu, Jul 11, 2019, 12:02 PM Jan Olaf Krems [email protected] wrote:

That sounds pretty promising! Thanks for digging into it, @GeoffreyBooth https://github.com/GeoffreyBooth. But I think one important question is

• would maintainers of a popular package add it and keep it even when stray node 6 users (with default npm) complain? Or maybe: When would it be realistic that they would keep it?

@jdalton https://github.com/jdalton Is this something you'd still move forward with, e.g. in lodash-es@5? Or is it too early to tell for you?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/modules/issues/348?email_source=notifications&email_token=AADZYV2VBU2QNNUPP2DFABLP65KSPA5CNFSM4H7PSUO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXFTIY#issuecomment-510548387, or mute the thread https://github.com/notifications/unsubscribe-auth/AADZYV2X3TTITINQITRT2MDP65KSPANCNFSM4H7PSUOQ .

[ljharb]

I think renaming the field is a much simpler solution. Updating npm isn’t always easy or possible, and effectively breaking those versions of npm for everyone as the ecosystem adopts this field seems too steep a price just for the word “type”.

[jdalton]

@jkrems

@jdalton Is this something you'd still move forward with, e.g. in lodash-es@5? Or is it too early to tell for you?

Yep, for sure in a major bump.

[GeoffreyBooth]

Yep, for sure in a major bump.

There’s another solution that could avoid a semver major bump, if your reason for wanting "type": "module" is to enable ESM in .js files. So for your existing package that already has CommonJS sources and a CommonJS file in "main", obviously we can’t make "main" point to an ESM file without a major bump. So we would put the ESM files in a subfolder, say /module, and also create a /module/package.json with "type": "module". Then you update the README to tell users to use pkg/module/index.js for ESM.

And once "exports" lands, the top-level package.json could map /module to /module/index.js to make the specifier a bit prettier. Either way, there’s no need for the type field in the top-level package.json.