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

Use PostCSS + PostCSS plugins for style transformation #49521

Merged
merged 11 commits into from
Oct 20, 2023

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Mar 31, 2023

What?

This PR uses PostCSS and PostCSS plugins for style transformations.

Why?

The internal CSS parser has compatibility and stability issues, as it is not a dedicated, separate library.

How?

Using PostCSS ensures that a dedicated and well-supported library is used.

Testing Instructions

  1. Use the Gutenberg plugin with this PR (branch) applied.
  2. Add frontend styles or other styles as editor stylesheet that are known to cause issues with the custom CSS parser (e.g. url([...]url([...])[...]) causes styles wrapping to fail completely (although valid CSS) #21569; Error while traversing the CSS: property missing ':' #19930 and from other issues).
  3. Open the Gutenberg editor,
    note that the editor styles are correctly post-processed and applied, resulting in correctly wrapped selectors and rewritten URLs.
  4. Add the HTML Preview block and check that adding custom styles do not break it.

Related PR: #49483

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 31, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @strarsis! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

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

@strarsis
Copy link
Contributor Author

Well, tests do not pass yet, some modifications have to be made.

Copy link
Member

@zaguiini zaguiini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gutenberg isn't building here, I'm not sure if I'm missing something:

  ERROR in ./node_modules/postcss-editor-styles/node_modules/postcss/lib/input.js 6:35-50
  Module not found: Error: Can't resolve 'path' in '/Users/zaguini/Development/gutenberg/node_modules/postcss-editor-styles/node_modules/postcss/lib'

  BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
  This is no longer the case. Verify if you need this module and configure a polyfill for it.

  If you want to include a polyfill, you need to:
  	- add a fallback 'resolve.fallback: { "path": require.resolve("path-browserify") }'
  	- install 'path-browserify'
  If you don't want to include a polyfill, you can use an empty module like this:
  	resolve.fallback: { "path": false }

The following diff fixes this problem:

diff --git a/tools/webpack/packages.js b/tools/webpack/packages.js
index 30f73a82fa..815dbf3a82 100644
--- a/tools/webpack/packages.js
+++ b/tools/webpack/packages.js
@@ -120,6 +120,11 @@ const vendorsCopyConfig = Object.entries( vendors ).flatMap(
 );
 module.exports = {
 	...baseConfig,
+	resolve: {
+		fallback: {
+			path: false,
+		},
+	},
 	name: 'packages',
 	entry: gutenbergPackages.reduce( ( memo, packageName ) => {
 		return {

Comment on lines 18 to 19
wrap( { scopeTo: wrapperClassName } ),
rebaseUrl( { rootUrl: baseURL } ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! It's very smart to use existing transformers instead of building in-house ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though the rebaseUrl had to be created by me, as there are no plugins yet that do exactly that.

@zaguiini
Copy link
Member

Is there a way to expose a CSS traverse function? I think it's possible with postcss.parse. There are use cases where the user might want to transform CSS (e.g. inline styles directly from Gutenberg).

@zaguiini
Copy link
Member

zaguiini commented Mar 31, 2023

For some reason, PostCSS is adding >114kb to the final bundle 😕

It looks like the wrap function is to blame:

image

Maybe we could use the in-house transformers to save those KBs and then expose the CSS traversal function just like we're doing right now with traverseCSS. This is how we could implement it:

image

return postcss( [
wrap( { scopeTo: wrapperClassName } ),
rebaseUrl( { rootUrl: baseURL } ),
] ).process( css, {} );
Copy link
Member

@zaguiini zaguiini Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.process returns a LazyResult (Promise). Reference: https://postcss.org/api/#processor-process

Well, this is unfortunate... We need to make this sync And this is how we can make it a sync operation: postcss/postcss#1328 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the sync variant should also work.

@strarsis
Copy link
Contributor Author

strarsis commented Apr 1, 2023

Is there a way to expose a CSS traverse function?

Yes, PostCSS has traversal functions, so it can be exposed.

wrap function is to blame

There is an alternative PostCSS plugin that has the required features but should be much more lightweight:
postcss-prefixwrap.

@strarsis
Copy link
Contributor Author

strarsis commented Jul 18, 2023

So I run into some roadblocks while making this PR mergeable: npm install resulting in error cb() never called!.
Strangely, invoking npm install with --verbose flag fixes the issue (which, as a side note, is a bit reminiscent of the progress bar performance issue in the earlier days of npm).
I can not use a newer node or npm version as those are locked in the package.json (also those version ranges should work, hence it should also work on my system).

Using the last commit that passes the CI tests (69a1573), a few tests fail on my workstation.
At the beginning of the test script I get this message:

The prop value with an expression type of JSXFragment could not be resolved. Please file an issue to get this fixed immediately.

A few tests fail:

Test Suites: 6 failed, 584 passed, 590 of 594 total
Tests:       2 failed, 1 skipped, 7183 passed, 7186 total
Snapshots:   4 obsolete, 345 passed, 345 total
Time:        631 s

The test hangs at this point.

These are the npm scripts I ran before the actual test script:

npm run fixtures:regenerate
npm run pretest:unit:php
npm run pretest:unit:php:multisite

Am I missing a npm script that is necessary for the tests, so they all pass on my workstation?
Or can I skip some tests that would not run on CI anyway?

@zaguiini
Copy link
Member

Wow, weird error indeed @strarsis.

@gziolo @youknowriad do you have any idea on how to fix this problem?

@strarsis I also see there's a conflict now with trunk. Maybe fixing the conflict and trying to run NPM again might fix it?

@strarsis
Copy link
Contributor Author

strarsis commented Jul 18, 2023

@zaguiini: I first tried to let the tests of the PR pass, which did not work fully.
Now I try to get a commit working which CI tests should pass, but this also does not work.
At least I can be sure that something with the tests is flaky, as the CI test passes.

Edit: The tests finally finished:

Test Suites: 6 failed, 585 passed, 591 of 594 total
Tests:       12 failed, 1 skipped, 7193 passed, 7206 total
Snapshots:   1 failed, 14 obsolete, 2 written, 350 passed, 353 total
Time:        12652 s

Three and a half hours is a bit excessive... But the 19 failed tests are the main issue.

Addendum: After resetting everything the tests now ran for 5 minutes, which is much better. Though some are still failing.

@strarsis
Copy link
Contributor Author

Alright, apparently the tests ran at commit are not the same as the one by the npm test script.
As committing works (again), I should be able to finish this PR.

@strarsis
Copy link
Contributor Author

strarsis commented Jul 20, 2023

Hm, some checks fail. I have to find out, why those failed.
postcss-prefixwrap is now used, which does not have the dependency issues.

@strarsis
Copy link
Contributor Author

I would really like to get this passing and merged.

Why does that E2E test fail, is this actually caused by the changes in this PR?
https://github.com/WordPress/gutenberg/actions/runs/5607873039/job/15192526172#step:7:60614

@zaguiini
Copy link
Member

@gziolo @jsnajdr @youknowriad could you please help @strarsis here?

@jsnajdr
Copy link
Member

jsnajdr commented Aug 14, 2023

Why does that E2E test fail, is this actually caused by the changes in this PR?

@strarsis Yes, there are React errors reporting that a LazyResult object is being passed as a React child:

Objects are not valid as a React child (found: [object LazyResult]). If you meant to render a collection of children, use an array instead.

The LazyResult is the promise-like return value of the postcss() call. It needs to be awaited before being used as a value. Or the postcss call needs to be a sync one. The problem was already discussed here (#49521 (comment)) but apparently not fixed yet.

Generally, the idea of this PR is interesting -- bundling and using the postcss package in Gutenberg code, and running it in the browser. I didn't know this is even possible, I always thought that postcss is just a Node.js a build tool.

@strarsis strarsis changed the title Use PostCSS + PostCSS plugins for style transformation [Type] Enhancement Use PostCSS + PostCSS plugins for style transformation Aug 14, 2023
@strarsis strarsis changed the title [Type] Enhancement Use PostCSS + PostCSS plugins for style transformation Use PostCSS + PostCSS plugins for style transformation Aug 14, 2023
@strarsis
Copy link
Contributor Author

@jsnajdr: Thanks for the pointer!
An error occurs: No label was set for the PR.

@jsnajdr jsnajdr added the [Package] Block editor /packages/block-editor label Aug 29, 2023
@jsnajdr
Copy link
Member

jsnajdr commented Aug 29, 2023

Hi @starsis 👋 As the last step, can you please rebase the PR onto the latest trunk and resolve conflicts? There is 5 months worth of changes in trunk since this PR was initially opened.

@zaguiini
Copy link
Member

zaguiini commented Sep 25, 2023

@strarsis do you have time to work on this issue? If not, I'd appreciate writing permissions to your repo so that I could push changes too.

@strarsis
Copy link
Contributor Author

strarsis commented Sep 25, 2023

@zaguiini: Sure! Sadly I had been too busy. I sent you an invitation.

@zaguiini
Copy link
Member

zaguiini commented Sep 26, 2023

@strarsis @jsnajdr how would you test it? Should we add some of the transformer tests that were removed (please provide examples) and adapt to the transformStyles function? I think it would be beneficial to ensure we're not breaking anything, but at the same time, would we be testing implementation details?

I also noticed that some PHP tests are failing. Surely, it's flakiness, as the PR doesn't touch PHP files. But it'd be good to confirm.

@zaguiini zaguiini self-assigned this Sep 26, 2023
@zaguiini zaguiini enabled auto-merge (squash) October 20, 2023 17:15
@zaguiini zaguiini merged commit 33fabf5 into WordPress:trunk Oct 20, 2023
50 checks passed
@github-actions
Copy link

Congratulations on your first merged pull request, @strarsis! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 20, 2023
@zaguiini
Copy link
Member

@strarsis This PR is finally merged 🎉 🎉

Could you please close the related issues and PRs and communicate the folks that this is finally over? Thanks!

@strarsis
Copy link
Contributor Author

strarsis commented Oct 20, 2023

@zaguiini: Done! I also added the note to issues/PRs that are related to style isolation and discussed issues with the CSS parsing.

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 [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants