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

Fix RTLCSS Support #68456

Open
wants to merge 14 commits into
base: trunk
Choose a base branch
from
Open

Fix RTLCSS Support #68456

wants to merge 14 commits into from

Conversation

SmushyTaco
Copy link

@SmushyTaco SmushyTaco commented Jan 2, 2025

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.

This solves WordPress#68364 to allow for the proper use of control directives.
Copy link

github-actions bot commented Jan 2, 2025

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @SmushyTaco.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Unlinked contributors: SmushyTaco.

Co-authored-by: gziolo <[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 github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

👋 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 gziolo added the [Tool] WP Scripts /packages/scripts label Jan 2, 2025
@gziolo
Copy link
Member

gziolo commented Jan 2, 2025

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?

@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Jan 2, 2025
@gziolo gziolo requested a review from t-hamano January 2, 2025 09:28
@SmushyTaco
Copy link
Author

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

@SmushyTaco
Copy link
Author

SmushyTaco commented Jan 2, 2025

@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
Copy link
Member

gziolo commented Jan 3, 2025

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
Copy link
Author

@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
Copy link
Author

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

@SmushyTaco
Copy link
Author

SmushyTaco commented Jan 4, 2025

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
Copy link
Author

SmushyTaco commented Jan 5, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripts: Control Directives for RTLCSS don't work in build command
2 participants