Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

chore: Fix mocha in Node v22+ #4701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 21, 2025
Merged

chore: Fix mocha in Node v22+ #4701

merged 2 commits into from
Feb 21, 2025

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Feb 20, 2025 •
edited
Loading

Alternative to #4629, should be much safer.

Basic gist is that require(esm) seemingly begins to parse modules to determine whether it's CJS or ESM, and in the case of the latter, require hooks are not yet supported against ESM modules so Babel doesn't transform the JSX within.

From Joyee's blog:

So that led to this PR. The major difference between this and the PR in 2019 was that this tried to keep the scope of require(esm) small and only supported loading synchronous ESM. As it turned out, this is not a controversial idea among collaborators/TSC at all and landed without much pushbacks (it derailed a tiny bit due to the discussions about loader hooks, but at least we agreed that hooks support could be left for follow-ups and require(esm) itself should be back on track).

https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/

@coveralls
Copy link

coveralls commented Feb 20, 2025 •
edited
Loading

Coverage Status

coverage: 99.609%. remained the same
when pulling 61d15bb on chore/node-22
into a54a914 on main.

marvinhagemeister
marvinhagemeister approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, would do a quick format because newlines are missing at the end.

@rschristian
Copy link
Member Author

rschristian commented Feb 21, 2025 •
edited
Loading

Do we want newlines? Our editor config disables those for .json files:

preact/.editorconfig

Lines 10 to 13 in 2a788d6

[{*.json,.*rc,*.yml}]
indent_style = space
indent_size = 2
insert_final_newline = false

Will check with biome shortly, not sure what it enforces by default

Edit: Our editorconfig & biome settings were indeed conflicting, I edited the editor config as most of our other files were missing line endings so it makes sense to match biome.

@rschristian rschristian merged commit d7b4787 into main Feb 21, 2025
4 checks passed
@rschristian rschristian deleted the chore/node-22 branch February 21, 2025 12:13
@JoviDeCroock JoviDeCroock mentioned this pull request Feb 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rschristian @coveralls @marvinhagemeister @JoviDeCroock
Morty Proxy This is a proxified and sanitized view of the page, visit original site.