Upgrade to use Node.js 20 and bump npm dependencies #2517

eason9487 · 2024-08-09T10:42:57Z

Changes proposed in this Pull Request:

Closes #2002

This PR:

Bump npm dependencies and upgrade to use Node.js v20.
Fix the build config of Fast Refresh mode.
Address Sass deprecation warnings.
- Ref: https://sass-lang.com/documentation/breaking-changes/mixed-decls/
Turn off a few eslint rules:
- react/react-in-jsx-scope rule
- jsdoc/check-line-alignment rule
- import/no-named-as-default rule
Resolve eslint errors: (💡 It would be easier to review the changes by individual commits)
Automatically fixed errors by running npm run lint:js -- --fix: c41ef64
@wordpress/i18n-text-domain rule: cdd9aab
jsdoc/no-undefined-types rule: 67ef0e2
import/no-unresolved rule: c4db2a3
no-redeclare rule: 6997b6f
jsdoc/require-yields-check rule: 47d1f9f
@typescript-eslint/no-use-before-define rule: 9886344
jsdoc/tag-lines rule: fccc0cd

❗ Since the jest testing requires a lot of adjustments, it will be handled by #2544. Two of the eslint rule errors are highly related to jest and are therefore handled by #2544 as well.

Detailed test instructions:

Set Node.js to v20
Run npm install to see if it can finish without errors
Check if the following scripts work well
- npm run build
- npm run dev
- npm run start
- npm run start:hot
- npm run lint:css
- npm run lint:js
  - There are two pre-existing warnings that still can not be fixed after this upgrade
  - Errors from the testing-library/no-unnecessary-act and testing-library/no-unnecessary-act rules are handled separately in Migrate jest tests to use Node.js 20 and make components compatible with upgraded packages #2544
Run node ./node_modules/.bin/bundlewatch to check if the check works
Do some smoke testing

Changelog entry

Dev - Upgrade to use Node.js 20 and bump npm dependencies.

Ref: https://sass-lang.com/documentation/breaking-changes/mixed-decls/

Fixed by running `npm run lint:js -- --fix`.

…e` rule.

puntope · 2024-08-21T10:05:05Z

Address Sass deprecation warnings.

I still see the deprecations in both this PR and in #2544

eason9487 · 2024-08-21T11:17:24Z

Hi @puntope

I still see the deprecations in both this PR and in #2544

The SCSS styles that caused deprecations came from @wordpress/base-styles and could not be resolved from this repo.

puntope

Howdy @eason9487 I tested all the scripts, code and did several smoke testing:

Onboarding
Edit campaigns
Create rules in attribute mapping
Campaign assets

Everything is working fine without errors so far.

I just left some comments regarding one CSS and just to be sure dependabot is compatible with the new node version.

puntope · 2024-08-22T09:10:58Z

js/src/components/contact-information/contact-information-preview-card.scss

-		@include placeholder;
-
 		display: inline-block;
 		width: 18em;
+
+		@include placeholder;


Is this change necessary? I wonder if placeholder includes display or width props. Then it will be overrided

Is this change necessary?

The change fixes a Sass deprecation warning. The explanation can be found in https://sass-lang.com/documentation/breaking-changes/mixed-decls/

I wonder if placeholder includes display or width props. Then it will be overrided

They don't have. Please refer to:

google-listings-and-ads/js/src/css/abstracts/_mixins.scss

Lines 9 to 21 in fccc0cd

@mixin placeholder($lighten-percentage: 30%) {

animation: loading-fade 1.6s ease-in-out infinite;

background-color: $gray-100;

color: transparent;

&::after {

content: "\00a0";

}

@media screen and (prefers-reduced-motion: reduce) {

animation: none;

}

}

puntope · 2024-08-22T09:17:43Z

js/src/components/free-listings/setup-free-listings/index.js

@@ -2,6 +2,7 @@
 * External dependencies
 */
 import { useRef } from '@wordpress/element';
+import { Form } from '@woocommerce/components';


Wondering if it's worth it to import the component just for the docs.

The main difference is the code editor's reference hints will be better, apart from that there shouldn't be any difference. So it should be worth it.

Without import

With import

puntope · 2024-08-22T09:27:31Z

package.json

-		"node": "^16 || ^18",
-		"npm": "^8 || ^9"


So I assume dependabot now supports

"node": "^20", and "npm": "^10"

?

Thanks for catching this!

I thought dependabot would use paired node and npm versions. Surprisingly, it's using unpaired Node.js 20 and npm 9. Even the npm version claimed by its error message is incorrect, bringing more confusion.

Dependabot uses Node.js v20.17.0 and NPM 10.8.2. Due to the engine-strict setting, the update will not succeed.

Fixed in b2a3d13

puntope · 2024-08-22T09:35:04Z

.eslintrc.js

 		'jest/expect-expect': [
 			'warn',
 			{ assertFunctionNames: [ 'expect', 'expect[A-Z]\\w*' ] },
 		],
+		// Turn it off temporarily because it involves a lot of re-alignment. We can revisit it later.
+		'jsdoc/check-line-alignment': 'off',
+		// The JS package `tracking-jsdoc` changes the definition of the `@fires` tag.


Can you elaborate what do you mean by this?

The JS package tracking-jsdoc changes the definition of the @fires tag.

Adjusted the code comment in c76514b. Let me know if it's still not clear enough.

I will just write "Listing our event names to avoid eslint false alarms"

But not a blocker tho.

…t pair with Node.js 20. Address: #2517 (comment)

…lint rule. Ref: #2517 (comment)

eason9487 · 2024-08-23T04:15:56Z

Hi @puntope, thanks for the review! This PR is ready for a new round of code reviews.

puntope

✅ LGTM

Thanks for the changes and all your work here

eason9487 self-assigned this Aug 9, 2024

github-actions bot added the changelog: dev Developer-facing only change. label Aug 9, 2024

eason9487 added 14 commits August 16, 2024 17:20

Bump npm dependencies and upgrade to use Node.js v20.

8d6d98b

Fix the build config of Fast Refresh mode.

b8b1129

Address Sass deprecation warnings.

8dc4a94

Ref: https://sass-lang.com/documentation/breaking-changes/mixed-decls/

Turn off the eslint rule react/react-in-jsx-scope.

ddc2b96

Turn off the eslint rule jsdoc/check-line-alignment.

f6997f7

Turn off the eslint rule import/no-named-as-default.

23536a4

Resolve eslint errors for the @wordpress/i18n-text-domain rule.

cdd9aab

Resolve eslint errors that can be automatically fixed.

c41ef64

Fixed by running `npm run lint:js -- --fix`.

Resolve eslint errors for the jsdoc/no-undefined-types rule.

67ef0e2

Resolve eslint errors for the import/no-unresolved rule.

c4db2a3

Resolve eslint errors for the no-redeclare rule.

6997b6f

Resolve eslint errors for the jsdoc/require-yields-check rule.

47d1f9f

Resolve eslint errors for the `@typescript-eslint/no-use-before-defin…

9886344

…e` rule.

Resolve eslint errors for the jsdoc/tag-lines rule.

fccc0cd

eason9487 force-pushed the dev/2002-use-nodejs-20 branch from 8e1905e to fccc0cd Compare August 16, 2024 11:56

eason9487 marked this pull request as ready for review August 19, 2024 11:02

eason9487 requested a review from a team August 19, 2024 11:02

eason9487 linked an issue Aug 19, 2024 that may be closed by this pull request

Upgrade to Node 20 #2002

Closed

4 tasks

puntope reviewed Aug 22, 2024

View reviewed changes

eason9487 added 2 commits August 23, 2024 11:28

Allow npm ^9 because dependabot is using an npm version that doesn'…

b2a3d13

…t pair with Node.js 20. Address: #2517 (comment)

Add a more detailed code comment to the jsdoc/no-undefined-types es…

c76514b

…lint rule. Ref: #2517 (comment)

eason9487 requested a review from puntope August 23, 2024 04:16

puntope approved these changes Aug 23, 2024

View reviewed changes

eason9487 merged commit 7316de3 into dev/use-nodejs-20 Aug 23, 2024
3 of 5 checks passed

eason9487 deleted the dev/2002-use-nodejs-20 branch August 23, 2024 11:09

eason9487 mentioned this pull request Aug 23, 2024

Upgrade to use Node.js 20 and make components compatible with upgraded packages #2562

Merged

tomalec mentioned this pull request Aug 27, 2024

Release 2.8.4 #2569

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to use Node.js 20 and bump npm dependencies #2517

Upgrade to use Node.js 20 and bump npm dependencies #2517

eason9487 commented Aug 9, 2024 •

edited

Loading

puntope commented Aug 21, 2024

eason9487 commented Aug 21, 2024 •

edited

Loading

puntope left a comment

puntope Aug 22, 2024

eason9487 Aug 23, 2024

puntope Aug 22, 2024

eason9487 Aug 23, 2024

puntope Aug 22, 2024

eason9487 Aug 23, 2024

puntope Aug 22, 2024 •

edited

Loading

eason9487 Aug 23, 2024

puntope Aug 23, 2024

eason9487 commented Aug 23, 2024

puntope left a comment

	@mixin placeholder($lighten-percentage: 30%) {
	animation: loading-fade 1.6s ease-in-out infinite;
	background-color: $gray-100;
	color: transparent;

	&::after {
	content: "\00a0";
	}

	@media screen and (prefers-reduced-motion: reduce) {
	animation: none;
	}
	}

Upgrade to use Node.js 20 and bump npm dependencies #2517

Upgrade to use Node.js 20 and bump npm dependencies #2517

Conversation

eason9487 commented Aug 9, 2024 • edited Loading

Changes proposed in this Pull Request:

Detailed test instructions:

Changelog entry

puntope commented Aug 21, 2024

eason9487 commented Aug 21, 2024 • edited Loading

puntope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eason9487 commented Aug 23, 2024

puntope left a comment

Choose a reason for hiding this comment

eason9487 commented Aug 9, 2024 •

edited

Loading

eason9487 commented Aug 21, 2024 •

edited

Loading

puntope Aug 22, 2024 •

edited

Loading