Skip to content
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.

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

Migrate deprecated 'assert' import syntax, bump to Node 18.20 #35826

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

fatso83
Copy link
Contributor

@fatso83 fatso83 commented Sep 11, 2024

Description

This fixes #35825 by ensuring that yarn start works. Currently this breaks on both Node 20 and 22 due to scripts using a mix of syntax only supported by either version.

Motivation

I wanted to contribute a tiny fix and this made it impossible to check the result by firing up yarn start (it still does, by the way, as the proxy feature doesn't seem to work on my Mac). Now, at least, I can run tests and linting.

Additional details

  • Remove deprecated 'assert' import syntax in favor of 'with'
  • Specify engines field to minimum Node 22 for experimental 'import ...with' support

Related issues and pull requests

Fixes #35825

Background
nodejs/node#51622

Note that with is already unflagged in 20 and 21 (and in 18 right now it just doesn't exist, not even behind a flag).

@fatso83 fatso83 requested review from mdn-bot and a team as code owners September 11, 2024 05:30
@fatso83 fatso83 requested review from bsmth and removed request for a team September 11, 2024 05:30
@github-actions github-actions bot added system [PR only] Infrastructure and configuration for the project size/s [PR only] 6-50 LoC changed labels Sep 11, 2024
@fatso83 fatso83 requested a review from a team as a code owner September 11, 2024 05:38
@fatso83
Copy link
Contributor Author

fatso83 commented Sep 11, 2024

After seeing that this PR failed in the Github workflow due to .nvmrc setting the Node version to 18.20.4, I installed that locally, and that worked. I could revert the engines change, but still, there is no documentation on which Node version to use in the docs (not even mentioning NVM), so that does not really fix anything. Also, Node 18 is reaching EOL in these days and it's about time to update the version anyway 😄

I'll await some feedback. I can for instance include a reference in the contributor docs to look at .nvmrc for the supported version.

@bsmth
Copy link
Member

bsmth commented Sep 11, 2024

Thanks for opening this one. We have a tracker for moving to Node 20 in MDN repos here: mdn/mdn#521

@fatso83
Copy link
Contributor Author

fatso83 commented Sep 11, 2024

According to the linked issue no one should start working on this yet. Should I then just revert all nvmrc and package.json changes, but keep the rest and explicitly set the engine to ">=18.18.0, < 19.0.0" to avoid issues such as the ones I got?

I could also make a mention in the contributing.md docs about referring to .nvmrc for which Node version to use?

@caugner
Copy link
Contributor

caugner commented Sep 11, 2024

Should I then just revert all nvmrc and package.json changes, but keep the rest and explicitly set the engine to ">=18.18.0, < 19.0.0" to avoid issues such as the ones I got?

We strive to support all current and future LTS versions of Node.js, and currently also support the active LTS version 18. So we don't want to restrict the engine to < 19.0.0.

The good news is that Node 18.20.0 added support for import attributes, so we should require >=18.20.0 instead and migrate the deprecated assert syntax.

@fatso83 Can you update your PR accordingly? 🙏

@fatso83
Copy link
Contributor Author

fatso83 commented Sep 11, 2024

Updates done

CONTRIBUTING.md Outdated Show resolved Hide resolved
This is the minimum version that supports the new `import ... with`
syntax.
@caugner caugner changed the title Fix failing yarn start and missing supported Node versions Migrate deprecated 'assert' import syntax, bump to Node 18.20 Sep 19, 2024
@caugner caugner merged commit aef1553 into mdn:main Sep 19, 2024
14 of 15 checks passed
@caugner
Copy link
Contributor

caugner commented Sep 19, 2024

Thanks @fatso83!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/s [PR only] 6-50 LoC changed system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocumented requirements on Node version for building the site
3 participants