Skip to content
This repository was archived by the owner on Sep 2, 2023. It is now read-only.
This repository was archived by the owner on Sep 2, 2023. It is now read-only.

type: module breaks users on various version of npm #348

Open
Open
type: module breaks users on various version of npm#348

Description

@jdalton
jdalton
opened on Jul 10, 2019

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

Activity

  1. devsnek commented on Jul 10, 2019

    @devsnek
    devsnek
    on Jul 10, 2019
    Member

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

  2. hybrist commented on Jul 10, 2019

    @hybrist
    hybrist
    on Jul 10, 2019
    Contributor

    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.

  3. ljharb commented on Jul 10, 2019

    @ljharb
    ljharb
    on Jul 10, 2019
    SponsorMember

    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?

  4. hybrist commented on Jul 10, 2019

    @hybrist
    hybrist
    on Jul 10, 2019
    Contributor

    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

  5. hybrist commented on Jul 10, 2019

    @hybrist
    hybrist
    on Jul 10, 2019
    Contributor

    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.

  6. robpalme commented on Jul 10, 2019

    @robpalme
    robpalme
    on Jul 10, 2019 via email
    Contributor

  7. guybedford commented on Jul 10, 2019

    @guybedford
    guybedford
    on Jul 10, 2019
    Contributor

    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).

  8. GeoffreyBooth commented on Jul 10, 2019

    @GeoffreyBooth
    GeoffreyBooth
    on Jul 10, 2019
    Member

    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.

  9. ljharb commented on Jul 10, 2019

    @ljharb
    ljharb
    on Jul 10, 2019
    SponsorMember

    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.

  10. hybrist commented on Jul 10, 2019

    @hybrist
    hybrist
    on Jul 10, 2019
    Contributor

    @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.

  11. jdalton commented on Jul 10, 2019

    @jdalton
    jdalton
    on Jul 10, 2019
    MemberAuthor

    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.

  12. GeoffreyBooth commented on Jul 11, 2019

    @GeoffreyBooth
    GeoffreyBooth
    on Jul 11, 2019
    Member

    I can reproduce the issue:

    ✦ docker run node:6.7.0 bash -c 'npm install lodash-es@4.17.13'
npm info it worked if it ends with ok
npm info using npm@3.10.3
npm info using node@v6.7.0
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" "lodash-es@4.17.13"
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 lodash-es@4.17.13'
npm info it worked if it ends with ok
npm info using npm@5.0.0
npm info using node@v8.0.0
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 lodash-es@4.17.13~preinstall: lodash-es@4.17.13
npm info linkStuff lodash-es@4.17.13
npm info lifecycle lodash-es@4.17.13~install: lodash-es@4.17.13
npm info lifecycle lodash-es@4.17.13~postinstall: lodash-es@4.17.13
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 lodash-es@4.17.13' 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.

  13. hybrist commented on Jul 11, 2019

    @hybrist
    hybrist
    on Jul 11, 2019
    Contributor

    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?

  14. MylesBorins commented on Jul 11, 2019

    @MylesBorins
    MylesBorins
    on Jul 11, 2019 via email
    Contributor

  15. ljharb commented on Jul 11, 2019

    @ljharb
    ljharb
    on Jul 11, 2019
    SponsorMember

    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”.

  16. jdalton commented on Jul 11, 2019

    @jdalton
    jdalton
    on Jul 11, 2019
    MemberAuthor

    @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.

  17. GeoffreyBooth commented on Jul 11, 2019

    @GeoffreyBooth
    GeoffreyBooth
    on Jul 11, 2019
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions