primus
[ESM] Incorrect default export, causing issue in Vite-powered projects when loaded as ESM
https://arethetypeswrong.github.io/?p=eventemitter3%405.0.1
In projects that uses Vite and use something like: https://github.com/antfu/vite-plugin-optimize-exclude, eventemitter3 is excluded from automatic deps re-bundling, and will be loaded as an ESM module directly.
But it seems like the export is not correct, and it will break, throwing: Requested module does not provide export named 'default'.
This seems fixed by #272. @lpinca would it be possible to get a release with this fix?
https://github.com/primus/eventemitter3/pull/272 forces "browsers" (and bundling for the browser) to use the ESM variant. I'm not sure if it is always wanted. It may break existing builds.
Ah, okay. I've been trying to ship a (non-bundled) ESM build of a library that depends heavily on EventEmitter3 and have been running into this issue - I ended up using a sed expression to post process the generated importmap from JSPM as a stopgap fix.
If you have a preferred fix for this issue I'd be happy to execute on it and open a PR?
I think the best movement would be to release a semver major in which we convert the package to TS and let the build tool automatically create correct CJS and ESM exports, heck, we can even make it ESM only and requires a higher node version in the new major, a lot of popular libraries from multiple authors have been doing this recently.
Edit: Actually for the higher node version part, now that I rethink about it, node is gonna have a native emitter soon and eventemitter3 would likely be used as polyfill for older node so we shouldn't require higher node.
I'm not sure how to fix this issue. I think using dist/eventemitter3.esm.min.js / dist/eventemitter3.umd.min.js for conditional exports in package.json works, but that is also a non-fix.
@lpinca I think for now we can try some small adjustments and check with @arethetypeswrong/cli to see if the import is fixed for a possible patch release?
I suspect there's a simpler solution than the nuclear option described above. I'll try to poke at this today or tomorrow.
I suspect there's a simpler solution than the nuclear option described above. I'll try to poke at this today or tomorrow.
We can try to fix the import/export, but because we're touching import/export it can be a dangerous thing so I think it might worth a major semver to try not breaking anything for current users.
FWIW I think the issue reported by https://arethetypeswrong.github.io/?p=eventemitter3%405.0.1 is a false positive. The change was done on purpose in https://github.com/primus/eventemitter3/commit/dd449775b872a340789dd29025a208ce80612959#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8R131-L134 and it seems correct as per https://github.com/primus/eventemitter3/blob/4e76a51d4af66f318e7d6b4f1c250ff05a51ff90/index.mjs#L4
Is it really false positive?, We are encountering invalid default export in applications just like the tool reports.
Browser like environments should import / require dist/eventemitter3.esm.js / dist/eventemitter3.umd.js instead of eventemitter3.
I agree that something seems weird with the output of that tool. Is it perhaps because the package is missing a module field that specifies the module index separately from the CJS index? It seems like that is duplicative information to the exports field, but I'm not sure how that tool works under the hood.
I've been looking into this on and off between meetings and can confirm that importing ./index.js in a browser ESM context does not work, so the existing index.mjs is definitely nonfunctional for browsers since it imports that file and re-exports it. Still poking at why that is and how to fix it.
Huh, TIL you literally can't mix CJS and ESM in the browser like you can in Node.js.
#278 fixes the issue by codifying @lpinca's recommendation above. Turns out you can specify both require and import under the browser key.