Dependency Updates and ESM Migration

[SmushyTaco]

This fixes #68364 to allow for the proper use of control directives.

What?

This PR stops the moving of comments and only removes comments that aren't RTLCSS control directives.

Why?

RTLCSS control directives use comments and if those comments are removed or moved, this will cause this to not function properly. See #68364 for more information.

How?

The issue is addressed by removing all comments that aren't RTLCSS control directives.

Testing Instructions

Build this SCSS code:

.wp-block-create-block-my-block {
  /*!tedgfdfg*/
  /*!rtl:ignore*/
  text-align: left;
  /*!rtl:remove*/
  background: #CCC;
}

Only the RTLCSS comments will be preserved as intended.

Testing Instructions for Keyboard

Not applicable.

Screenshots or screencast

Not applicable.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: SmushyTaco <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: shvlv <[email protected]>
Co-authored-by: Rishit30G <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @SmushyTaco! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[gziolo]

Thank you for opening this patch. It looks like it will fix the issue. Do you have any thoughts on how the comment could still be removed from the production file, as it's only necessary to instruct rtlcss?

[SmushyTaco]

@gziolo I think I found a better solution. I'll work on writing it and push a new commit once done.

[SmushyTaco]

@gziolo I have made the following changes:

1. Updated all dependencies in the package.json
2. eslint was only updated to 8.57.1 because 9.0.0 and beyond deprecates the eslintrc format that's currently used by this project.
3. Replaced cssnano with css-minimizer-webpack-plugin, this is the recommended way to apply cssnano with webpack and fixes the issue with RTLCSS.
4. Deleted the bundled rtlcss-webpack-plugin fork and used this one.
5. Replaced the clean-webpack-plugin package with this fork. This was done because my fork comes with performance improvements and updated dependencies to avoid deprecation warnings.
6. read-pkg-up was renamed to read-package-up for newer versions so that was changed.
7. chalk was replaced with picocolors which is 14 times smaller and 2 times as fast.
8. TODO messages were added for changes that need to be made in the future.
9. REGEX expressions were made more concise.
10. Replace merge-deep with ts-deepmerge, it's more robust and actively maintained.
11. Replace cwd with package-up because cwd is a 9 year old stale package.
12. Replace the use of fs.exists with the proper fs.access.
13. Remove the deprecated url-loader and replace it with webpack 5's built in functionality seen here.

There's only 1 caveat that needs fixing. When building, this message is seen:

(node:27960) ExperimentalWarning: CommonJS module C:\Users\organ\Documents\GitHub\gutenberg\packages\scripts\utils\package.js is loading ES Module C:\Users\organ\Documents\GitHub\gutenberg\node_modules\read-package-up\index.js using require().

Which I will be fixing soon.

[gziolo]

Replaced cssnano with css-minimizer-webpack-plugin, this is the recommended way to apply cssnano with webpack and fixes the issue with RTLCSS.

I see that the fix boils down to using an existing webpack plugin css-minimizer-webpack-plugin that runs cssnano after RTL version of the CSS file is created. Cool!

At the moment, CI jobs report multiple issues related to updated npm packages. In practice, it's usually easier to land targeted PRs that solve one problem at a time. For example:

eslint was only updated to 8.57.1 because 9.0.0 and beyond deprecates the eslintrc format that's currently used by this project.

@shvlv is working on an ESlint upgrade to v9 in #65648.

How did you test all changes applied to the @wordpress/scripts package?

[SmushyTaco]

@gziolo the tests seem to be failing from package-up and read-package-up's dependencies only supporting ES6 modules. I've forked the base modules to support CommonJS and will work on their dependencies now to fix this. In the future I'll make sure to keep 1 PR for 1 problem like you suggested. Thankfully the work is almost done with this PR.

Regarding how I've tested all of this, I'll go into depth on this once I make the finishing touches. It's currently 1:30 AM for me so I'll aim to wrap this up after I wake up.

[SmushyTaco]

I'll resolve the merge conflict once I'm home.

[SmushyTaco]

I'll make some finishing touches to wrap this up soon. I'll work on the clean-webpack-plugin fix when I wake up.

[SmushyTaco]

It looks like we need the ESLint 9 update to fix this (or wait for importing ESM modules with require() to no longer be experimental) so the tests pass. I hope my progress towards ESM is appreciated and understood as something that'll eventually be necessary for the future maintenance of this project.

In the meantime before the ESLint 9 migration, I'll investigate solutions.

This might work, I'll investigate.

[SmushyTaco]

@gziolo from what I'm seeing, it seems like the main issue is Jest not picking up the tests now that they're ESM. Jest's ESM support is still experimental and from what I see here, it seems like ESM migration is something you guys want in the future. Have you guys considered migrating away from Jest and to Vitest?

With all that said, I'm going to open a new PR just for the RTLCSS bug fix soon so we can get that merged ASAP.

This PR will focus on ESM migration and keeping dependencies up to date. It'll be able to be merged after the ESLint 9 migration and after tests are migrated to ESM.

[SmushyTaco]

See #68493

[shvlv]

Regarding ESLint v9, the last state is we support ^8.57.0 || ^9.11.0 in @wordress/scripts. So, ensuring we can still support (within Gutenberg and as a standalone tool) v8 here would be nice.

Regarding the PR scope, it's hard for me to understand why we need to switch to ESM modules and why only the scripts package; what about the rest of the packages? The same regarding multiple dependency updates at once, I agree with @gziolo:

In practice, it's usually easier to land targeted PRs that solve one problem at a time

[SmushyTaco]

@shvlv yeah given the added complexity I see why solving one problem at a time can simplify things. That's why I opened #68493 which focuses just on the RTLCSS fix like this one should've. I migrated to ESM on this branch because many of the packages used here became ESM only in later versions and ESM module support from CommonJS is still experimental. It might be better to just wait for that stabilize before updating dependencies rather than migrating to ESM right now which is a much larger undertaking. With that said, I'll be closing this PR in favor of #68493

If you could look at the new PR to see why the unit tests are failing that would be greatly appreciated. The changes are minimal and I'm not seeing anything related to the changes in the error logs, which makes me think these tests were failing before any of my changes. When I run npm run test:unit on trunk with the latest stable NodeJS version (v22.13.0) and the latest version of NPM (11.0.0) the unit tests have some fails too.

So with all that said, I think #68493 should be able to be merged now to fix support for RTLCSS.