fix: Enable generation of ESM so that it can be used with native imports #123
Conversation
| const Benchmark = require('benchmark'); | ||
| const SuperJSON = require('./dist/').default; | ||
| import Benchmark from 'benchmark'; | ||
| import SuperJSON from './dist/index.js'; |
There was a problem hiding this comment.
The reason to use require here was that it enabled running node benchmark.js, which is quite helpful in dev. Is this change neccessary?
There was a problem hiding this comment.
Running node benchmark.js fails in the latest node versions after adding "type": "module" if using require.
There was a problem hiding this comment.
Added a script to ensure the benchmark can be run in Node 10 as well, by enabling modules with the experimental flag. You can now run yarn benchmark.
| @@ -0,0 +1,23 @@ | |||
| { | |||
There was a problem hiding this comment.
I can't spot a difference to the previous config. What's the idea behind this change?
There was a problem hiding this comment.
Mentioned it in the description:
Moved .eslintrc.js to .eslintrc.json because some the older ESLint versions
tsdxdepends on would fail to require it now that the closest package.json is of "type": "module".
| "typings": "dist/index.d.ts", | ||
| "type": "module", |
There was a problem hiding this comment.
I like the addition of ESM exports!
package.json
Outdated
| @@ -11,14 +13,15 @@ | |||
| "node": ">=10" | |||
| }, | |||
| "scripts": { | |||
| "build": "tsc", | |||
| "clean": "rm -rf ./dist", | |||
| "build": "tsup src/index.ts --dts --sourcemap --target es5 --format cjs,esm", | |||
There was a problem hiding this comment.
Could we make this work with TSC itself, too? We can't drop the Node 10 CI.
There was a problem hiding this comment.
I'll take a look and see if I can achieve the same output using tsc alone.
Have in mind that it's only building in Node 10 that fails, using the library in Node 10 should work.
There was a problem hiding this comment.
Added a fallback to tsc in Node 10 since tsup uses the TextEncoder global which was added in Node 12.
There was a problem hiding this comment.
Wonder why Node.js 10 support is still required, it's reaching the end of life next month
There was a problem hiding this comment.
(just removed TextDecoder from tsup since it's not necessary)
There was a problem hiding this comment.
Node.js is pre-ES6 and thus a good proxy for testing legacy browser support.
|
Hey @ElMassimo, this PR makes quite some big changes to our build process. If it's okay for you, I'd like to close it in favour of #127, which takes a leaner approach. |
|
@Skn0tt Sounds good! I don't mind as long as I can import this library as a module 😃 |
|
@allcontributors add ElMassimo for code |
|
I've put up a pull request to add @ElMassimo! 🎉 |
Description 📖
This pull request modifies the
buildcommand to also output ESM, so that this library can be used with native ESM imports.Background 📜
Currently, the library is specifying a
"dist/superjson.esm.js",that is not being generated, so native imports fail.Notes ✏️
Using
tsupinstead oftsc, which has better defaults.Unfortunately,
tsupdoes not support older node versions, so we usetscwhen doing CI with Node 10.Moved
.eslintrc.jsto.eslintrc.jsonbecause some the older ESLint versionstsdxdepends on would fail to require it now that the closestpackage.jsonis of"type": "module".