WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10k stars 4.02k forks source link

Expose `@emotion/styled` to extenders #61232

Open fabiankaegy opened 2 months ago

fabiankaegy commented 2 months ago

Several of the ´@wordpress/´ packages use @emotion/styled internally as their CSS in JS solution.

When 3rd party block component packages then try to also use the same library first of all it causes a lot of duplicate code to be loaded slowing down the editor. But on top of that emotion also adds the following warning in the browser console:

[!Warning]

You are loading @emotion/react when it is already loaded. Running multiple instances may cause problems. This can happen if multiple versions are used, or if multiple builds of the same version are used.`

Proposed solution

It would be geat to either be able to import the functions used by the components package like useCx styled etc. in external packages.

Alternatively maybe @emotion/styled & @emotion/react could get bundled as their own packages in WordPress core similar to how we have react bundled in core.

In the end I think the ideal solution would be to not directly expose emotion or any other CSS in JS library but instead expose the abstracted layer that allows anyone to use the library without having to worry about whether a component gets rendered inside an iframe or not etc. Just making it easy to use the same component styling system that core already is using internally :)

fabiankaegy commented 2 months ago

@WordPress/gutenberg-components I'd love to get your take on this one.

Essentially we are running into many issues working on the @10up/block-components package with having to duplicate the efforts that are already handled by core here and would love to simplify that setup to reduce the bloat that needs to get introduced.

fabiankaegy commented 2 months ago

The __experimentalStyleProvider is already getting exported here: https://github.com/WordPress/gutenberg/blob/e2142c3e6a7562ef9e9e059f8c85a9c6791212f6/packages/components/src/index.ts#L202 but without the other pieces of emotion it sadly isn't very helpful

mirka commented 2 months ago

I'm afraid we're planning on moving off of Emotion, and more generally any kind of CSS-in-JS library that does any runtime work. The three main motivators being that we've been finding it ultimately unnecessary, we're starting to see actual performance costs, and the bundle size cost that it adds.

I'm curious though, does this affect an extender's decision to adopt Emotion in their projects? Even though we intentionally weren't exposing Emotion functions because we didn't want to commit to endorsing it publicly, was the fact that wp-components was using Emotion under the hood encouraging extenders to use it too?

I also would like to know if you've been encountering use cases where Emotion is necessary, rather than simply being convenient. Or, which parts of Emotion do you find the most value in.

fabiankaegy commented 2 months ago

Thanks for those insights @mirka :)

To give a bit of insight, we would also in theory find a way to get rid of CSS in JS. But the thing that is keeping us there is that the way we distribute our component library is as an NPM package. And so far we've not found a way to easily ship a stylesheet alongside the library without adding a lot of additional effort to the consumers or the library to manually enqueue the CSS on all the editor screens and in all the framed and not iframed editors.

Since we've not found a solution for that we needed to look at CSS in JS solutions and then started adopting emotion because core also used it. Ideally we want all our components to he built in a way where they could be brought over to core if we find that there is a use-case for it :)

DaniGuardiola commented 2 months ago

@fabiankaegy I'm not yet entirely familiar with the full context of this problem space, but I've observed that many UI libraries have standardized on using CSS imports which, while not "spec" standard, are pretty standard indeed in practice and a basic feature of any bundler out there (they will also be closer to "spec" standard when import attributes land). It's also definitely the simplest approach.

Even if we want to support a bundle-less solution, we could engineer a very simple "singleton" insertion mechanism as a fallback, and with CSS Cascade layers (which I'm gonna push for) style precedence won't even be an issue then. Or, as a last resort, we could just shift the responsibility of importing styles to the user which is typically trivial.

A full CSS-in-JS approach is not necessary in any case, and I feel like it'd be trivial to implement a "singleton" approach as I described above with useInsertionEffect or a more vanilla approach.

Of course, I'm just thinking out loud here and none of this is normative, but this is one of the things I plan to focus on in the near future.

Tangentially, the overall trend I'm seeing is towards less wrappers and re-exports, e.g. we're now encouraging direct imports of React rather than the @wordpress/elements package: https://github.com/WordPress/gutenberg/issues/54074 (thanks @mirka for sharing it in another thread)

fabiankaegy commented 2 months ago

Hey @DaniGuardiola 👋 Thanks also for your insights :)

I am completely on board with everything you are saying. But I'm still not sure how a component library would be able to actually include some CSS that gets enqueued properly inside and outside the iframed editor without creating a bunch of work for the user of the component library.

That is the main hurdle which keeps us using a CSS in JS solution. Because we can handle the logic for where the stylesheet gets injected for the consumer of the library.

DaniGuardiola commented 2 months ago

@fabiankaegy what do you mean by "where the stylesheet gets injected"? Are you referring to the order relative to other stylesheets that might or might not override the styles? Or just about the process of loading them in the page, regardless of order?

Also, I'm not sure I'd call using any bundler that supports CSS imports (all of them pretty much) "a bunch of work". It's a pretty standard / out-of-the-box thing these days and almost required for a sane bundle size (minification also observably speeds up parsing and execution of JavaScript). This also only affects those consumers that use the components package specifically, which is a smaller subset, so I feel like requiring it is reasonable.

Of course, there's the matter of backwards compat, which might be the thing that renders this approach ultimately not possible, but I think it's worth considering at least, as well as potential fallbacks as I shared earlier :)

fabiankaegy commented 2 months ago

@DaniGuardiola What I'm talking about is how the styles actually get enqueued in WordPress.

If you want to load some JS in any of the editors today you need to enqueue it via wp_enqueue_script on the enqueue_block_editor_assets hook. This takes care of loading that JS in all the editors in WordPress. So today when someone uses any of our react components in the editor all they have to do is import the component into that JS file and they are good to go.

If we were to not use a CSS in JS solution the consumer of our library would now also need to manually import the CSS somewhere and enqueue it on both the enqueue_block_editor_assets and enqueue_block_assets hook with a conditional to only load it if is_admin is true. The reason for this is that the JS may portal into the editor canvas iframe but also place elements outside the iframe. So the CSS needs to be loaded in both contexts.

As far as I'm aware there is no simple way to do all of this as the publisher of an NPM package on behalf of the user if there isn't a runtime level thing happening to inject the styles wherever the component is used.

In a webpack context having a import "@10up/block-components/dist/style.css" would only result in the CSS getting output in an additional CSS file which I now manually need to worry about

mirka commented 2 months ago

@youknowriad @ellatrix Any way we can make this easier for third-party component library consumers without having them rely on runtime CSS-in-JS solutions? From the current state of dev ex it does indeed sound like CSS-in-JS is the most convenient here, which is unfortunate for overall performance.

fabiankaegy commented 2 months ago

The one alternative I have considered quite a few times is instead of distributing the component library via NPM, it could also get distributed as a WordPress plugin. And therefore handle all the enqueueing manually same as WordPress core does for all the bundled packages.

Consumers of these library would then need to add the library as a script dependency in WP and that should take care of everything.

But shipping it as a plugin comes with quite a lot of additional overhead and other problems...

DaniGuardiola commented 2 months ago

Thinking outside the box a bit here but, what if we "added support for CSS imports" in those hooks in some way? Seems like some very basic regex matching (in lack of actual AST parsing) could be good enough to pull this off:

  1. Find CSS imports in JavaScript assets being loaded.
  2. Remove from JS before loading.
  3. Load corresponding stylesheets as assets.

Then, we can support both use-cases in a standard way. Win win?

youknowriad commented 2 months ago

I'm not entirely sure I understand all the nuance of all the comments here but here's what I'm understanding:

The main issue @fabiankaegy is raising is the fact that npm packages have a hard time exposing CSS and JS at the same time while allowing a good DevX import MyComponent from 'my-library and get everything working by default.

This is obviously an issue that is common in the JS community and not really a Gutenberg or a WordPress issue. In fact, as you can see in our "Gutenberg As A Framework" website https://wordpress.org/gutenberg-framework/docs/intro, it's an issue for npm packages shipped by Core as well. Third-parties need to import these CSS files manually.

When it comes to npm packages, my opinion is that we should not introduce something in the output that is not standard yet. Sure CSS imports are widely used but I believe we should stick with standards and this is why Core packages ship one or multiple CSS files at the moment (or use some CSS in JS).

Now, for importing CSS into the WordPress block editor by third-party developers, I do believe we need to offer a way to mark some CSS as "content" which means the output gets loaded into the iframe/previews... and some other CSS as regular stylesheet (UI editor CSS). I believe @ellatrix has worked on this before, I'm pretty sure there are ways to do that already. Maybe we're lacking some documentation?

fabiankaegy commented 2 months ago

@youknowriad I agree that this issue exists in any JS application. However for WordPress builds it is a little more complex still because we have to load these styles both in the iframe and outside the iframe.

Most of the time developing custom blocks you don't really need to be aware of the whole iframe stuff. That is a lower level thing that just works. So it is asking quite a bit of a library consumer to ensure that the library styles are manually enqueued everywhere.

Yes there are APIs to load styles inside and outside of the iframe. (enqueue_block_editor_assets / enqueue_block_assets). But when someone is just developing a block where all they do is adding their editorScript via block.json, the last thing you want to do is require them to do is manually add some PHP in the main plugin / theme code to enqueue some library CSS in a few different contexts.

Because of all this complexity we currently are pretty locked into using a CSS in JS solution. But because core is doing the same thing without exposing it to be used by others we have to load it multiple times with possibly different versions which causes all kinds of issues and bloat.

I totally get that core doesn't want to get locked in to some API like this. I'm just pointing out what issues we are facing and what few alternatives we currently have :)

DaniGuardiola commented 1 month ago

Folks, I think the solution we were looking for just fell from the sky and into our hands :)

https://react.dev/reference/react-dom/components/style#special-rendering-behavior

I now think the best plan of action is wait until this is officially supported and we are in a version of React that supports it, and then just use inline styles handled and de-duplicated by React. Basically, CSS-in-JS but without any libraries, future-proof, minmal bundle size and maximal performance.

I'm not super-familiar with the inside/outside iframe styles situation, but if Emotion worked then I'm fairly confident this approach will also work, since it fundamentally does the same (inject some de-duplicated inline styles).

Thoughts?

tyxla commented 1 month ago

In theory that sounds like it could solve it. However, I'm a bit concerned about how it would handle the style tag order.

Imagine loading component X and component Y, and their styles come with conflicting precedence - they're using the same selector and providing a different value for a particular style property. There is a chance that the value will depend on the load order of the components - if X renders before Y, Y's styles will be used, and the other way around. Perhaps React has found a way to always have the same stylesheet order, but that sounds like an impossibly tricky problem to solve, especially as you move styles around, create new stylesheets, and remove old ones.

FWIW currently in Calypso, we have similar issues with lazy-loaded components when precedence is not well sorted out.

DaniGuardiola commented 1 month ago

@tyxla

image

Plus style precedence should be handled through CSS Cascade Layers nowadays, a native CSS feature supported in all browsers for that exact use-case. In the components package, specifically, I already have a draft proposal that will suggest using them to fix these issues you're referring to in a clean manner.

DaniGuardiola commented 1 month ago

Also, to be clear, replacing emotion with this React API (and something like precedence="low") would instantly improve things today. Emotion actually appends all styles at the end, taking precedence over any other loaded styles. This is the reason why I've had to use !important in the past to force overrides of Emotion-provided styles.

DaniGuardiola commented 1 month ago

Another relevant note is that there's already some alignment around using layers for other matters within WordPress: https://github.com/WordPress/gutenberg/discussions/61810

ellatrix commented 1 month ago

We have style overrides useStyleOverride, but it's not currently a public API. I'm ok with making the hook public, but very hesitant with making the private store action public. Does the hook solve your problem?

tyxla commented 1 month ago

@tyxla

image

Plus style precedence should be handled through CSS Cascade Layers nowadays, a native CSS feature supported in all browsers for that exact use-case. In the components package, specifically, I already have a draft proposal that will suggest using them to fix these issues you're referring to in a clean manner.

@DaniGuardiola I guess that assumes rewriting all existing styles to use cascade layers? If that's what you mean, won't that cause compatibility issues with existing plugins and themes that rely on overriding these styles the old way?

DaniGuardiola commented 1 month ago

won't that cause compatibility issues with existing plugins and themes that rely on overriding these styles the old way?

What do you mean by "the old way"? Unlayered styles always have priority over layered styles, so unless I'm missing something those overrides should still work.

tyxla commented 1 month ago

Most styles across plugins and themes will be unlayered, so yes, that's what I mean by the old way. What you're saying makes sense, although I'm not 100% convinced all custom 3rd party CSS will work the same way. Also, a migration could be interesting since it may cause some styles to be layered, and some not, and that might lead to unexpected precedence issues.