Bump wp-scripts to 22.1.0 and webpack config to v5 #1339

eason9487 · 2022-03-18T09:24:33Z

Changes proposed in this Pull Request:

It's the stage 1 of #1333

💡 There are a lot of stylelint fixes. It should be easier to review with

Upgraded packages

@wordpress/scripts
- Specify the semantic versioning as ~22.1.0 to only accept patch releases because version 22.2.0 seems to have a potential breaking change.
@wordpress/dependency-extraction-webpack-plugin
@wordpress/prettier-config
@wordpress/stylelint-config
postcss
prettier
stylelint

🔒 Since eslint also extends the version and config of @woocommerce/eslint-plugin, it is currently not upgradeable. And this brings some new workarounds to the codebase.

Breaking changes

Webpack building

Handle the path (path-browserify) polyfill of native node.js module for postcss .
- Add path-browserify package.
Handle the process polyfill of native node.js module for @wordpress/compose that @wordpress/components depends on.
Add browserslist config to package.json to avoid the IE11 incompatible warnings when building CSS.
- It also drops support for Internet Explorer 11
Filter out the CopyPlugin (CopyWebpackPlugin) plugin from the default webpack config of @wordpress/scripts.

`wp-scripts` commands

lint:js - Fix the path resolving problem of .js files and a module.
lint:css - Fix the breaking change that stylelint v14 no longer includes SCSS syntax.
- Add stylelint-config-standard-scss package.
test:js - Update the dependent packages of jest and a related testing library.
- Upgrade @testing-library/react
- Add jest-environment-jsdom

⚠️ lint:md:js was originally broken and seems to have not been in use, so this PR won't fix it.

Detailed test instructions:

Checkout tweak/1333-stg1-upgrade-wp-scripts branch.
npm run install
Except for packages-update and lint:md:js, run all wp-scripts related commands to see if they work properly.
- Should be executed without error
  - npm start
  - npm run dev
  - npm run build
  - npm run lint:css
  - npm run lint:js
  - npm run test:js
  - npm run test:js:watch
  - npm run format:js
  - npm run check-engines
- Can be executed but errors are detected because the current repo/codebase does not match their checks
  - npm run lint:md:docs
  - npm run lint:pkg-json
  - npm run check-licenses
Look around the GLA pages, it should work fine and the DevTool console has no exception errors.

Additional details:

The nanoid package to be fixed in #1353 should be resolved with the merging of this PR into develop.

Changelog entry

Tweak - Upgrade @wordpress/scripts version to 22.1.0 and other packages that depend on it.
Tweak - Upgrade webpack config version to v5.

… on it

… for IE11)

…ting library

…ncludes SCSS syntax

…ig of `@wordpress/scripts`

puntope

❓ / 🚧

When I run the Tests in my IDE they fail. If I roll back to develop, it works normally.

Example of command:

/Users/miguelperezpellicer/.nvm/versions/node/v14.19.0/bin/node --require /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-stdin-fix.js /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/node_modules/jest/bin/jest.js --colors --reporters /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-reporter.js --verbose --testNamePattern=^Performance Card  --runTestsByPath /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/js/src/dashboard/summary-section/performance-card.test.js

result:

Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'cwd')

      at Object.getCacheKey (node_modules/@wordpress/scripts/node_modules/babel-jest/build/index.js:299:33)

puntope · 2022-03-22T17:03:54Z

❓ WDYT about fixing package.json to prevent errors in npm run lint:pkg-json

https://github.com/woocommerce/google-listings-and-ads/compare/tweak/1333-stg1-upgrade-wp-scripts...add/1333-package-json?expand=1

puntope · 2022-03-22T17:08:57Z

❓/ 💅 npm run lint:md:docs

Seems like this is checking as well vendor folder???? 😕 But in the documentation it says is not checking it.

WDYT about removing it?

tomalec · 2022-03-24T13:15:13Z

❓ / 🚧
When I run the Tests in my IDE they fail. If I roll back to develop, it works normally.

FWIW, 😕 Works on my end

tomalec · 2022-03-24T13:15:50Z

❓ WDYT about fixing package.json to prevent errors in npm run lint:pkg-json
https://github.com/woocommerce/google-listings-and-ads/compare/tweak/1333-stg1-upgrade-wp-scripts...add/1333-package-json?expand=1

📜 I'd rather make it a separate PR, to make both easier to review.

tomalec · 2022-03-24T13:17:25Z

❓/ 💅 npm run lint:md:docs
Seems like this is checking as well vendor folder???? 😕 But in the documentation it says is not checking it.

WDYT about removing it?

📜
Seems like a bug/outdated docs in wp-scripts I'd rather rise an issue and fix it in Gutenberg repo.

https://github.com/WordPress/gutenberg/blob/trunk/packages/scripts/scripts/lint-md-docs.js#L47
https://github.com/WordPress/gutenberg/blob/trunk/packages/scripts/config/.markdownlintignore

I can volunteer to do that.

edit: done at WordPress/gutenberg#39724

tomalec · 2022-03-24T15:37:39Z

question / construction

When I run the Tests in my IDE they fail. If I roll back to develop, it works normally.

Example of command:

/Users/miguelperezpellicer/.nvm/versions/node/v14.19.0/bin/node --require /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-stdin-fix.js /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/node_modules/jest/bin/jest.js --colors --reporters /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-reporter.js --verbose --testNamePattern=^Performance Card  --runTestsByPath /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/js/src/dashboard/summary-section/performance-card.test.js

The reported line is:

        transformOptions.config.cwd,

Is it possible that your IDE does not load jest config properly?

I got the same result when I tried running in CLI:

node node_modules/jest/bin/jest.js  --colors  --verbose --testNamePattern=^Performance Card  --runTestsByPath js/src/dashboard/summary-section/performance-card.test.js

Does running just npm run test:js from CLI work on your machine? I guess that's why we use wp-scripts test-unit-js not jest directly.

tomalec · 2022-03-24T16:15:03Z

I guess it's a mismatch of config vs jest version.
node node_modules/jest/bin/jest.js --show-config gives 25.5.4. While npm run test:js -- --show-config gives 27.5.1 for this branch and 26.6.3 for develop.

Can you try set up your IDE to use npm run test:js instead of jest directly. So, it will actually make use of wp-scripts as all the other parties (package.json, CI, etc.) do?
Alternatively, we can try to sync those versions, but then I bet it would be a recurring problem, and for sure you will loose any benefits of wp-scripts test-unit-js

tomalec · 2022-03-25T09:59:11Z

❔ BTW, @eason9487 why do we need to

Filter out the CopyPlugin (CopyWebpackPlugin) plugin from the default webpack config of @wordpress/scripts.

tomalec

FWIW, I tested locally, reviewed the code changes, LGTM.

So the question is how much it is a deal-breaker for @puntope . We do not depend or use jest directly, and testing via wp-scripts works (at least for me).

I guess we can solve it on your end either via setting up IDE, or bumping @woocommerce/e2e-* packages that install the lower version of jest, but that's yet another can of worms, which I'd move to a separate PR.

… config. Address #1339 (comment)

eason9487 · 2022-03-25T10:55:42Z

@tomalec, thanks for the review, investigation, suggestions, and upstream reporting and fixing! 🙏

❔ BTW, @eason9487 why do we need to

Filter out the CopyPlugin (CopyWebpackPlugin) plugin from the default webpack config of @wordpress/scripts.

The main reason is that CopyPlugin will cause any changes to files in the src folder to trigger an unwanted webpack rebuild. It doesn't affect the building results, but it's a bit distracting. Also, although we don't have any block.js files, the fact that the behavior of automatically copying them to the build folder still worries me a bit.

I added a code comment to remedy the reason in 6dd5777.

eason9487 · 2022-03-25T11:18:25Z

@puntope, thanks for the review!

❓ WDYT about fixing package.json to prevent errors in npm run lint:pkg-json

I agree with this #1339 (comment). Or we could also fix it in the clean-up stage.

Although GPL-2.0 or later is suggested by WordPress linter, I'm not sure if the original license is for any reason, and this repo has third-party partner involvement. So I think there must be more sufficient confirmation before changing the license from GPL-3.0-or-later to GPL-2.0-or-later.

❓ / 🚧
When I run the Tests in my IDE they fail. If I roll back to develop, it works normally.

I did some surveys and got the same thoughts as @tomalec's investigation in #1339 (comment) and #1339 (comment).

I tried on my local env and checked the JavaScript Unit Tests job on GitHub Actions, they both work correctly. Probably needs more surveys on your end.

puntope · 2022-03-28T08:59:54Z

question / construction
When I run the Tests in my IDE they fail. If I roll back to develop, it works normally.
Example of command:
/Users/miguelperezpellicer/.nvm/versions/node/v14.19.0/bin/node --require /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-stdin-fix.js /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/node_modules/jest/bin/jest.js --colors --reporters /Applications/PhpStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-reporter.js --verbose --testNamePattern=^Performance Card  --runTestsByPath /Users/miguelperezpellicer/Sites/wp16-test/wordpress/wp-content/plugins/google-listings-and-ads/js/src/dashboard/summary-section/performance-card.test.js
The reported line is:
        transformOptions.config.cwd,
Is it possible that your IDE does not load jest config properly?

I got the same result when I tried running in CLI:
node node_modules/jest/bin/jest.js  --colors  --verbose --testNamePattern=^Performance Card  --runTestsByPath js/src/dashboard/summary-section/performance-card.test.js
Does running just npm run test:js from CLI work on your machine? I guess that's why we use wp-scripts test-unit-js not jest directly.

Yes, is mainly a problem of IDE. npm run test:js works perfectly fine. I will check around my IDE config. Thanks

eason9487 self-assigned this Mar 18, 2022

eason9487 requested a review from a team March 18, 2022 12:05

eason9487 marked this pull request as ready for review March 18, 2022 12:05

eason9487 added 10 commits March 21, 2022 16:49

Upgrade @wordpress/scripts to 22.1.0 and other packages that depend…

8bc62aa

… on it

Handle breaking change of webpack building - postcss

2ab71b3

Handle breaking change of webpack building - @wordpress/compose

9168af7

Handle breaking change of webpack building - browserslist (polyfill…

c7965c9

… for IE11)

test:js - Update the dependent packages of jest and a related tes…

9f10bdb

…ting library

lint:js - Fix the path resolving problem of .js files and a module

b51c8dc

lint:css - Fix the breaking change that stylelint v14 no longer i…

959b6ad

…ncludes SCSS syntax

Fix eslint errors

8a3aed5

Fix stylelint errors

b30f3a7

Filter out the CopyWebpackPlugin plugin from the default webpack conf…

a455f0a

…ig of `@wordpress/scripts`

eason9487 force-pushed the tweak/1333-wp-scripts-webpack branch from 34ca73d to 0167ec4 Compare March 21, 2022 08:52

eason9487 force-pushed the tweak/1333-stg1-upgrade-wp-scripts branch from ae8e464 to a455f0a Compare March 21, 2022 08:56

puntope reviewed Mar 22, 2022

View reviewed changes

tomalec approved these changes Mar 25, 2022

View reviewed changes

Add code comment about the reason of removing CopyPlugin from webpack…

6dd5777

… config. Address #1339 (comment)

puntope approved these changes Mar 28, 2022

View reviewed changes

eason9487 merged commit 6c6c857 into tweak/1333-wp-scripts-webpack Mar 28, 2022

eason9487 deleted the tweak/1333-stg1-upgrade-wp-scripts branch March 28, 2022 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump wp-scripts to 22.1.0 and webpack config to v5 #1339

Bump wp-scripts to 22.1.0 and webpack config to v5 #1339

eason9487 commented Mar 18, 2022 •

edited

puntope left a comment

puntope commented Mar 22, 2022

puntope commented Mar 22, 2022

tomalec commented Mar 24, 2022

tomalec commented Mar 24, 2022

tomalec commented Mar 24, 2022 •

edited

tomalec commented Mar 24, 2022 •

edited

tomalec commented Mar 24, 2022

tomalec commented Mar 25, 2022

tomalec left a comment

eason9487 commented Mar 25, 2022

eason9487 commented Mar 25, 2022

puntope commented Mar 28, 2022

Bump wp-scripts to 22.1.0 and webpack config to v5 #1339

Bump wp-scripts to 22.1.0 and webpack config to v5 #1339

Conversation

eason9487 commented Mar 18, 2022 • edited

Changes proposed in this Pull Request:

Upgraded packages

Breaking changes

Webpack building

wp-scripts commands

Detailed test instructions:

Additional details:

Changelog entry

puntope left a comment

Choose a reason for hiding this comment

puntope commented Mar 22, 2022

puntope commented Mar 22, 2022

tomalec commented Mar 24, 2022

tomalec commented Mar 24, 2022

tomalec commented Mar 24, 2022 • edited

tomalec commented Mar 24, 2022 • edited

tomalec commented Mar 24, 2022

tomalec commented Mar 25, 2022

tomalec left a comment

Choose a reason for hiding this comment

eason9487 commented Mar 25, 2022

eason9487 commented Mar 25, 2022

puntope commented Mar 28, 2022

eason9487 commented Mar 18, 2022 •

edited

`wp-scripts` commands

tomalec commented Mar 24, 2022 •

edited

tomalec commented Mar 24, 2022 •

edited