🧟 v3.0.2 #233
🧟 v3.0.2 #233
Conversation
There was a problem hiding this comment.
Looks pretty good; excellent work, thank you!
I've only looked at the diff/commits so far, i.e. haven't actually executed any code yet - but will have to do all that anyway as part of the release process (which always scared me a little, even back when I was still familiar with those shenanigans... ).
| @@ -8,15 +8,13 @@ let { nodeResolve } = require("@rollup/plugin-node-resolve"); | |||
|
|
|||
| let MODULE_FORMATS = { // maps faucet identifiers to Rollup identifiers | |||
| esm: true, | |||
| es: "esm", // alias | |||
| es6: "esm", // alias | |||
There was a problem hiding this comment.
I honestly have no idea why we had those aliases in the first place; "esm" probably wasn't firmly established back then? 👴
| umd: true, | ||
| amd: true, | ||
| commonjs: "cjs", | ||
| cjs: false, // deprecated in favor of `commonjs` | ||
| iife: true | ||
| }; | ||
| let NAMELESS_MODULES = new Set(["es", "amd", "cjs"]); // NB: Rollup identifiers | ||
| let NAMELESS_MODULES = new Set(["esm", "amd", "cjs"]); // NB: Rollup identifiers |
There was a problem hiding this comment.
Just to be sure: This should always have been "esm" as far as Rollup (not faucet) was concerned?
There was a problem hiding this comment.
This is my understanding, yes!
package.json
Outdated
| @@ -19,17 +19,17 @@ | |||
| "scripts": { | |||
| "test": "npm-run-all lint --parallel test:unit test:cli", | |||
| "test:cli": "./test/cli/run", | |||
| "test:unit": "mocha test/unit/test_*.js", | |||
| "test:unit": "node --test test/unit/test_*.js", | |||
There was a problem hiding this comment.
This is pretty cool - thank you for caring!
Just to be safe: Does this actually execute the same amount of tests? (I'm afraid I can't check myself at this time.)
| "test:unit": "node --test test/unit/test_*.js", | |
| "test:unit": "node --test ./test/unit/test_*.js", |
There was a problem hiding this comment.
Yes, it executes all tests. I also made a test fail to make sure the exit code is correct. Will fixup this change 👍
|
@FND Adressed all your feedback, and force pushed the change 👍 Let me know if anything is missing. |
it appears this was never correct in the first place
as suggested by @moonglum: this appears to be unmaintained, but it's not really worth the encumbrance anyway
|
@moonglum: I've added a couple of commits in preparation for the release. Note that I opted for v3.0.1 instead of v3.1, though I'm not entirely sure about that... Please note the newly added change log (very similar to faucet-pipeline/faucet-pipeline-core#147). Failing tests are expected because we're referencing pre-release versions (due to the weirdness of our DIY monorepo here). I've also allowed myself to rewrite history while I was reviewing commits for the change log, mostly to adjust commit messages' grammar for consistency. |
|
Thanks for getting this ready for release 👍 No objections to making this a 3.0.2 release instead of a 3.1 release. npm-run-all is definitely not a reason against it, as it is a dev dependency. So why would anyone depending on this package care? The only reason would be dropping support for Node 18. But I'm okay with that. |
Indeed, but that would be a major rather than a minor version change then? So yeah, 3.0.2 it is. |
|
Great, then I think this one is ready for release 👍 |
Re-energized by Node finally supporting real compatibility between ESM and CommonJS, I wanted to see how much work it is to update everything to the latest versions. Doesn't seem to be a lot:
The one dependency I did not update to the latest version is
@rollup/plugin-commonjs. Version 27 changes the output in a weird way.Then I cherry-picked two more changes from the swc branch:
esandes6)NAMELESS_MODULESusedesinstead ofesm