Skip to content

Conversation

@moonglum
Copy link
Member

@moonglum moonglum commented Jan 26, 2025

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:

  • I adjusted the test output, as browser support for newer features has gotten better (who knew!)
  • Then I removed mocha, as we can use the built-in test runner with no code changes
  • Updated the dependencies to the latest versions with one exception (all without any code changes)
    • The updates also remove the deprecation warnings you see in the version of faucet-js that is currently released
  • Updated the node support matrix (still supporting v18 in this release to make it non-breaking. Furthermore, it is still supported until April and there is no reason not to do it)

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:

  • Drop support for undocumented module format names (es and es6)
  • Bugfix: NAMELESS_MODULES used es instead of esm

@moonglum moonglum requested a review from FND January 26, 2025 10:04
Copy link
Contributor

@FND FND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... ).

let MODULE_FORMATS = { // maps faucet identifiers to Rollup identifiers
esm: true,
es: "esm", // alias
es6: "esm", // alias
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly have no idea why we had those aliases in the first place; "esm" probably wasn't firmly established back then? 👴

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly

iife: true
};
let NAMELESS_MODULES = new Set(["es", "amd", "cjs"]); // NB: Rollup identifiers
let NAMELESS_MODULES = new Set(["esm", "amd", "cjs"]); // NB: Rollup identifiers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure: This should always have been "esm" as far as Rollup (not faucet) was concerned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my understanding, yes!

package.json Outdated
"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",
Copy link
Contributor

@FND FND Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Suggested change
"test:unit": "node --test test/unit/test_*.js",
"test:unit": "node --test ./test/unit/test_*.js",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it executes all tests. I also made a test fail to make sure the exit code is correct. Will fixup this change 👍

@moonglum
Copy link
Member Author

@FND Adressed all your feedback, and force pushed the change 👍 Let me know if anything is missing.

Copy link
Contributor

@FND FND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

moonglum and others added 5 commits February 1, 2025 11:42
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
@FND FND force-pushed the 3.1 branch 2 times, most recently from 54d2013 to 7780cf2 Compare February 1, 2025 10:50
@FND
Copy link
Contributor

FND commented Feb 1, 2025

@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.

@FND FND changed the title 🧟 v3.1 🧟 v3.0.1 Feb 1, 2025
@moonglum
Copy link
Member Author

moonglum commented Feb 1, 2025

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.

@FND FND changed the title 🧟 v3.0.1 🧟 v3.0.2 Feb 2, 2025
@FND
Copy link
Contributor

FND commented Feb 2, 2025

The only reason would be dropping support for Node 18.

Indeed, but that would be a major rather than a minor version change then? So yeah, 3.0.2 it is.

@moonglum
Copy link
Member Author

moonglum commented Feb 2, 2025

Great, then I think this one is ready for release 👍

FND added 2 commits March 2, 2025 21:25
note that we don't actually _need_ faucet-core v3 though because `await`
doesn't care whether there's a promise or not
@FND
Copy link
Contributor

FND commented Mar 2, 2025

@moonglum: updated to account for faucet-core v3 see c880cac - though we'll have to update the change log after deciding on a version number

@moonglum
Copy link
Member Author

moonglum commented Mar 4, 2025

Thanks! I would probably still set to core v3 only. We need to make sure that all pipelines use the same version of faucet, otherwise things break. I would therefore set them all to require core 3.

Update: Let's discuss that aspect in the core PR.

==================================


v3.0.2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will go for 3.1.0, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants