Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

package.json "module": "./stable.esm.js" breaks stuff #9

Closed
simllll opened this issue Apr 17, 2018 · 15 comments · Fixed by #10
Closed

package.json "module": "./stable.esm.js" breaks stuff #9

simllll opened this issue Apr 17, 2018 · 15 comments · Fixed by #10

Comments

@simllll
Copy link

simllll commented Apr 17, 2018

I'm using accept-language node module which includes stable like this:
var stable = require("stable");

My whole project is bundled and packaged with webpack 4.5, and since this new release it includes require like this:
var stable = webpack_require(/*! stable */ "./node_modules/stable/stable.esm.js");

but unfortanutenaly this leads to "stable is not a function" errors.I guess the problem is that it should actually be a "import stable from.." statemetn instead of the regular requrie to let this work correctly. Or rewrite the stable call to stable.default(args...). I guess there are more situations like this though, and removing the "module" line in package.json fixes everything.

Not quite sure what is the correct approach to handle this. All I know: it doesn't work right now, and stable is the only package in my repro right now that has this kind of issue?

@simllll
Copy link
Author

simllll commented Apr 17, 2018

proposed solution: revert the module change (or all changes), and republish it as "1.x".. or any other "breaking" version number. then includes like "stable": "^0.1.6" will not update to the newer one, but newer libraries can do so if they want to.

@stephank
Copy link
Member

stephank commented Apr 17, 2018

Thanks for reporting this, sorry it came up!

I'm not sure what the correct solution here is, though. accept-language will always expect to import the commonjs stable, but is getting an ES6 module instead.

// This export in our ES6 module:
function stable () { /* ... */ }
export default stable

// Looks like this to the webpack commonjs world, such as `accept-language`:
module.exports = { default: stable }

// Hence the 'stable is not a function' error

Even if we publish the ES6 module changes as (breaking) semver 0.2, it'll just break again when accept-language upgrades to our 0.2.

So, I'm trying to figure out what to do here. Maybe @MattiasBuelens has an idea?

@stephank
Copy link
Member

I just noticed, accept-language is a TypeScript project, and sets __esModule:

Object.defineProperty(exports, "__esModule", { value: true });

Could you try temporarily removing that line @simllll, just to see if the build works that way?

@MattiasBuelens
Copy link
Contributor

stable uses generated UMD bundle as its main entry point for CommonJS loaders and the ESM bundle as its module entry point for ESM loaders. However, it seems that somehow your webpack build resolves a CommonJS-style require with an ESM module.

Where can I find your webpack config? Or could you provide a small reproduction case? Or maybe the relevant part of your webpack config so I can try and reproduce the issue?

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Apr 17, 2018

Okay, I managed to reproduce this with a minimal Webpack project that uses accept-language. This indeed throws an error when accept-language tries to call stable:

Uncaught TypeError: stable is not a function
    at AcceptLanguage.parse (AcceptLanguage.js:154)
    at AcceptLanguage.get (AcceptLanguage.js:55)
    at component (index.js:9)
    at eval (index.js:14)
    at Object../src/index.js (bundle.js:392)
    at __webpack_require__ (bundle.js:20)
    at eval (webpack:///multi_(:8080/webpack)-dev-server/client?:2:18)
    at Object.0 (bundle.js:403)
    at __webpack_require__ (bundle.js:20)
    at bundle.js:69

Other bundlers insert interopability helpers for handling ESM modules that were transpiled to CommonJS. For example, Babel does this:

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
var _foo = require("foo");
var _foo2 = _interopRequireDefault(_foo);

and Rollup does this:

foo = foo && foo.hasOwnProperty('default') ? foo['default'] : foo;

However, Webpack doesn't seem to do this (webpack/webpack#4742). I'll check if there's anything a library can do to make this work without needing interopability helpers.

@MattiasBuelens
Copy link
Contributor

The best I could come up with is this comment:

For library authors: As a workaround, you could:

  • offer a separate package like lodash does (lodash-es), or
  • skip the module field and use a es-specific entry point, like import a from 'yourpkg/es'
  • skip the module field, avoid es modules (my preferred solution for most of my modules)

The first two choices will be explicit and won't depend on tooling (but they'll need to be documented)
The last choice just works with all CommonJS loaders/tools.

So it looks like a library simply cannot use the module field if it wants to support require() in Webpack...

@simllll
Copy link
Author

simllll commented Apr 17, 2018

Thanks guys for looking into this. I also stumpled across the comment @MattiasBuelens just posted. "Solution" number two seems very legit to me though.
Remove "module" and add in the readme how to include the es specific module by including stable.esm?

Regards
Simon

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Apr 17, 2018

I found another silly workaround, which is to add a browser entry point:

"browser": "./stable.js"

By default, Webpack prefers the browser entry point over module or main entry points. In this case, it would select the UMD bundle instead of the ES bundle, which "fixes" the issue. Still, not recommended. 😛


Anyway, for v0.1.x, I think we should just remove module and not ship any ES modules just yet. Users expect that require('stable') gives them a function, not a namespace with a default property. We should not break that, even though other bundlers handle the interop better. (This is how idb-keyval "solved" the same problem.)

For a future v0.2.x release, we could consider a new public API that supports CJS and ESM. For example, rather than adding inplace as an expando property on the main stable function, we could have:

export default stable;
export { inplace };

This would mean that the CJS exports also change:

exports.default = stable;
exports.inplace = inplace;
exports.__esModule = true;

However, I'm not sure how bundlers handle mixing default exports with named exports... Perhaps we should just switch everything over to named exports:

export { stable, inplace };

and CJS:

exports.stable = stable;
exports.inplace = inplace;
exports.__esModule = true;

Side node: it seems like Node will use .mjs as file extension for ES modules, so we should probably rename stable.esm.js to stable.mjs when we re-introduce ESM.

@simllll
Copy link
Author

simllll commented Apr 19, 2018

Any plans? Right now I'm kinda stuck with this.

@netpoe
Copy link

netpoe commented Apr 25, 2018

Any success with this issue? I just came across the same problem using the https://github.com/ipld/js-ipld-dag-pb library as a dependency for the js ipfs package.

Using Angular ^5.2.0
ipfs ^0.28.2

ERROR TypeError: sort is not a function

When using it like this:

const sort = require('stable')

const sortedLinks = sort(links, linkSort)

Any ideas?

@BrettHoutz
Copy link

For my own project, implemented in TypeScript, the change to the typings in stable 0.1.17 is a breaking change. My project imports it like this:

import * as sort from 'stable';

tsc now generates the error message:

Cannot invoke an expression whose type lacks a call signature. Type 'typeof "<my project>/node_modules/stable/index"' has no compatible call signatures."

Changing it to a default import fixes it:

import sort from 'stable';

I think it would be best to incorporate Mattias's PR at least until 0.2+.

@netpoe
Copy link

netpoe commented Apr 25, 2018

I worked around it by forcing the stable package version to 0.1.6 in my package.json dependencies:

"stable": "0.1.6",

@stephank
Copy link
Member

Sorry for the delay. I merged the PR and published 0.1.8.

Still think this situation is weird, though. But I suppose we'll have to look into doing another release with one of the solutions mentioned by Mattias.

@MattiasBuelens
Copy link
Contributor

@stephank Maybe also deprecate the 0.1.7 release with npm deprecate stable@0.1.7, to help inform users about the bug and how to fix it.

@stephank
Copy link
Member

Done!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants