-
Notifications
You must be signed in to change notification settings - Fork 18
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
Update node to version 20 #340
base: master
Are you sure you want to change the base?
Conversation
This update is driven due to the fact that the peer dependency `eslint-plugin-jsdoc` needs to be updated to support node 20.
"@wordpress/scripts": "^19.2.3", | ||
"@wordpress/scripts": "^26.19.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear the potential side-effects of updating this package. The fact that this repository doesn't have CI checks gives low confidence in introducing changes. In any case, I presume that this package is mainly used for the development workflow, so probably won't introduce regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each plugin is manually released separately, so there at least wouldn't be any immediate problems if one of them no longer builds properly.
I ran yarn everything
to build all the plugins, and didn't see any issues that didn't exist before this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you very much @ajlende for the information and for checking that all plugins can be built 🙇 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blocks that I maintain here look fine: Bauhaus Centenary, Event, Model Viewer, Starscape, and Waves.
It looks like the waves and book blocks are a bit broken, but it's probably because they're using old __experimental
APIs in Gutenberg which have been deprecated and removed, not because of the changes in this PR.
I also had to make sure that I ran a manual yarn clean
before yarn start
for development because there were some extra files floating around causing errors.
It's probably worthwhile having someone more familiar with the layout grid test that one out since it's more widely used. But I'll give it one check of approval for the things that I maintain.
"@wordpress/scripts": "^19.2.3", | ||
"@wordpress/scripts": "^26.19.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each plugin is manually released separately, so there at least wouldn't be any immediate problems if one of them no longer builds properly.
I ran yarn everything
to build all the plugins, and didn't see any issues that didn't exist before this change.
Thanks @ajlende for testing some of the blocks. Following #340 (comment), since each plugin is released individually, in potential next releases we'd need to test if any regressions have been introduced.
Ah, interesting. I didn't experience this but I presume that changing the node version or dependency updates might be related.
Ok, thanks 🙇 ! I'll share a list with the remaining blocks to check and reference the last contributors who worked on them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As shared in #340 (comment), seems that all blocks can be built. However, I noticed that all unit tests are failing. Some of them don't pass either in master
but before merging this PR, we should at least fix the unit tests that were succeeding before this change.
The error I encountered is the following:
The error below may be caused by using the wrong test environment, see https://jestjs.io/docs/configuration#testenvironment-string.
Consider using the "jsdom" test environment.
ReferenceError: window is not defined
at Object.<anonymous> (node_modules/@wordpress/jest-preset-default/scripts/setup-globals.js:2:14)
Following #340 (review), I tried to address the issue when running tests but seems more complex than I originally thought. In the newer version of
Until this gets resolved, I think we should pause this PR so I'm going to revert it to draft. As a follow-up, I'll open an issue with this information (#342). |
This PR is a follow-up on the Gutenberg PR WordPress/gutenberg#56331 to update Node to the same version used in Gutenberg.
More information can be found in:
https://make.wordpress.org/core/2023/12/20/updating-wordpress-to-use-more-modern-versions-of-node-js-npm-2/
The dependencies update has been performed by running the following commands:
nvm use 16
yarn upgrade @wordpress/scripts --latest
nvm use 20
yarn upgrade