ai / nanoid

A tiny (124 bytes), secure, URL-friendly, unique string ID generator for JavaScript
https://zelark.github.io/nano-id-cc/
MIT License
24.35k stars 790 forks source link

nanoid@3.1.24 breaks browserify #295

Closed achingbrain closed 3 years ago

achingbrain commented 3 years ago

The "browser" override field in package.json points to an ESM index.browser.js that's only ESM on npm while being CJS on github?

Anyway - looks like you need to declare "type": "module" in package.json if you export ESM:

SyntaxError: 'import' and 'export' may appear only with 'sourceType: module' (4:0) while parsing /home/travis/build/ipfs/js-ipfs/node_modules/nanoid/index.browser.js while parsing file: /home/travis/build/ipfs/js-ipfs/node_modules/nanoid/index.browser.js
    at DestroyableTransform.end [as _flush] (/home/travis/build/ipfs/js-ipfs/node_modules/insert-module-globals/index.js:114:21)
    at DestroyableTransform.prefinish (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:138:10)
    at DestroyableTransform.emit (events.js:400:28)
    at prefinish (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:619:14)
    at finishMaybe (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:627:5)
    at endWritable (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:638:3)
    at DestroyableTransform.Writable.end (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:594:41)
    at DestroyableTransform.onend (/home/travis/build/ipfs/js-ipfs/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:577:10)
    at Object.onceWrapper (events.js:519:28)
    at DestroyableTransform.emit (events.js:412:35)

Something worth considering is using ipjs to do the transpiling & publishing - you can author in ESM and it'll publish a module that can be consumed seamlessly from ESM and CJS environments, including browserify, jest and a bunch of other weird/old/edge cases.

goto-bus-stop commented 3 years ago

browserify does not support ESM at all, so "type": "module" does not make a difference for that.

The "browser" field mapping for index.cjs should target a CommonJS file, instead of an ES module as it does now: https://github.com/ai/nanoid/blob/b460981ba90b04d45416f2b9e47efdfe3459af53/package.json#L23

oh, I see what you said about it being CJS on github but ESM on npm…

achingbrain commented 3 years ago

browserify does not support ESM at all

Fair point. If this change was desired it should really have gone out as a major.

achingbrain commented 3 years ago

Actually, looking back, index.browser.js has been ESM since 3.1.8.

The change seems to have been the upgrade of dual-publish from 1.0.5 to 1.0.6.

The published browser field has changed from:

  // 3.1.23
  "browser": {
    "./index.js": "./index.browser.js"
  },

to:

  // 3.1.24
  "browser": {
    "./index.js": "./index.browser.js",
    "./index.cjs": "./index.browser.js",
    "./async/index.js": "./async/index.browser.js",
    "./async/index.cjs": "./async/index.browser.js"
  },
goto-bus-stop commented 3 years ago

the change in the browser field was necessary, since previously, a CJS bundle included the entirety of crypto-browserify instead of using browsers' builtin crypto features. there needs to be an ESM version and a CJS version of the browser implementation of the module to make it work right in all situations.

ai commented 3 years ago

/cc @arturi

arturi commented 3 years ago

@ai haha, I was about to ping you, actually :) dual-publish assumes ESM for the browser, but some bundlers / projects still use CJS. We use browserify and CJS still in https://github.com/transloadit/uppy, planning to move to ESM in the future, but not yet.

Do you think dual-publish could support CJS for the browser too? https://github.com/mikeal/ipjs claims to support that, it seems?

ai commented 3 years ago

We need to be sure that webpack, Rollup, Parcel and ESM CDNs like JSPM will use package.exports (since we must to use ESM there).

Can you check?

arturi commented 3 years ago

Can you check?

I’m sorry, but my head melted a bit from trying to understand how dual-publish works, modules are not my strong suit. I think you have a better grasp on this. And regardless, we need a CJS build for the browser, right? Without crypto module require.

arturi commented 3 years ago

nanoid could be esm only but right now it maps a CJS entry point to an ES module which is wrong regardless of target platform it only works in things like webpack because they (intentionally) do not conform to spec

ai commented 3 years ago

Hope this changes in dual-publish will fix the problem https://github.com/ai/dual-publish/commit/0a4cebd07adde92f6ee4a924009c71ac13054a00

Unfortunately, it is very hard to find solution both for webpack and Browserify

ai commented 3 years ago

I will release new Nano ID with dual-publish 1.0.7 after the lunch (in case of any emergency since Nano ID is very popular and used in very different environments)

ai commented 3 years ago

Please check Nano ID 3.1.25 with the fix for Browserify

achingbrain commented 3 years ago

That appears to have fixed it, thanks for the speedy response

arturi commented 3 years ago

Same on my end, that for the speedy fix indeed!