-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Revisiting the new component system integration strategy #29689
Comments
During the WordPress core JS chat a few weeks back, we discussed that the first step would be to make an official proposal to recommend including G2 Components in WordPress. I would prefer it happens before we start more aggressive integration. There are so many components that you could already use and replace existing components and many others that would benefit the UI of Gutenberg. On the technical level, I’d like to see a different solution for RTL support rather than |
@ItsJonQ has a fix in the works for this: ItsJonQ/g2#278
Okay, after some discussion with @gziolo the current proposed plan is to remove automatic RTL support from the library altogether and go back to manual RTL like we were doing for CSS-in-JS in Gutenberg before (using the
I was under the impression this was in the works... @ItsJonQ is that still something you're working on? |
Here what I think, I know it's at odds with what your opinions.
|
Thanks for your input Riad 🙂 I appreciate our difference in opinions!
FWIW this is an inevitability given the current constraints on the adapter pattern that I laid out. Parts of core and parts of plugins, if they wish to take advantage of the next generation of WordPress components will have to be re-written.
Definitely not suggesting this. I think we should continue with the adapter strategy for backwards compatibility, but again it has significant limitations that I don't see any ways around other than directly exposing the new components somehow.
What is your vision for this happening? How do we swap out an adapter strategy for a fully replaced component? I agree this would be ideal but I'm not seeing how we'll ever be able to remove older APIs in favor of new, more accessible and intuitive ones? Understood that we may want to replace-in-place, I'm just not seeing how we're setting ourselves up for doing that today considering that references to Maybe there is a way we can think of that will be able to maintain versioning in a more coherent way rather than version A OR B? Like if we always have ComponentSystemProvider but it tells you what version of each component you're using, rather than trying to adapt <Button usesVersion1Api />
<ComponentSystemProvider versions={{ Button: 2 }}>
<Button usesVersion2Api /> // no adapter
</ComponentSystemProvider> Something like that? |
This is the crux of the issuue. I don't know why it's inevitable and why it should be inevitable. I know it's easy to offer two implementations (two components or a version flag) but for me, we should do our best to make it possible to switch from one implementation to the other without necessarily having to touch the consuming code (touching the consuming code should be an exception). I'm not saying this is easy, trust me I know it's not, it's very hard but it's easier than asking all the consumers to change their code. |
What's acceptable for me is:
What's not acceptable for me
|
Is this completely unacceptable, even in the case of say the Popover component where the new API guarantees accessibility concerns? I'll need to defer to Q for whether some of this is even possible. I simply don't see a world in which we can swap out |
Can you detail more here? Why wouldn't be able to do that? |
I detailed this in the issue description:
Maybe Q and I just didn't spend enough time trying to make it work (we might need some other more knowledgable brains to help us) but this presented a real difficult problem when we tried to use the adapter to wrap the top bar in Gutenberg. There are also new idioms like Just want to reiterate that this is a very difficult problem. I've spent some time thinking about it and haven't found any acceptable solution but hopefully more folks thinking about it can help discover some way of solving this problem 🙂 |
Would this be solved if the component keeps using the same classname and keeps accepting the "className" prop? |
Why would this be the case? I don't know the new system, so I'm asking why these components have so high specificy? |
The components do indeed keep the same classnames as the previous implementation (they do this automagically in fact so it is a guarantee we do not have to manage 🙂) however the specificity of those classnames is trounced by the specificy compounding of the new component system. Additionally all of the new components accept a className prop as a standard we are following (along with forwarding refs and etc). These guarantees are all encapsulated in the I'd like to try an experiment in getting rid of the specificy compounding that the new component system uses. I'll make a PR to potentially create an environment variable for it so that we can experiment. Maybe the specificy compounding isn't actually as necessary as we originally thought. It was originally added to combat the disparate styles across Gutenberg that affect components in different ways, but we may just have to rely on this old behavior if we want to make adaption possible. Thanks for pointing this out, I'll do some digging and see what I can find in this area. |
If the "preview" button is doing a lot of custom stuff we should try to remove that custom stuff since we have full control over it, no? |
I guess, the idea here is that third-party usage are also doing the same custom stuff and if we make it work with ours, it's going to work for third-party usage too. |
Following the conversation here, maybe I can delineate a few different issues that we could handle separately:
|
I went ahead and opened a PR that uses a low specificity version of the new component system. It looks like it fixes a LOT of the problems we were seeing: #29732 As it stands, I think that solves one of the problems I brought up here, but not all of them. I still think we need a better deprecation strategy than what we have now. |
Yeah! The Toolbar approach was pretty much writing a separate component inside the existing Toolbar component. That was enabled by an experimental prop ( {/* legacy toolbar */}
<Toolbar />
{/* accessible toolbar, renders a different component */}
<Toolbar label="label" /> Something similar is done on the SlotFill components with the Another thing to keep in mind is that this also caused some confusion with class names. For example: #29247 (comment) However, in my experience, creating separate components instead of adding custom props results in a code that's much easier to maintain. I wrote something related to this a few years ago and I maintain the same opinion. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I've given some more thought to this and I think what my desire boils down to is this: I want to be able to consume the new component system directly without adapters while also maintaining adapters for the old components to the new components so that we can automagically update UIs using the older components to use the new components. The problem with the current approach is that the adapters will forever mediate a developers access to the new component system in the cases where they overlap. This shouldn't be acceptable to us. For example, the current API for Button is potentially nonsense. What does it mean when you do
So... what it comes down to is this, I think it's very important to be able to access the new component system directly, raw, without any adapters mediating. I think the adapters are however important for us to create and maintain as well as the Furthermore, this will allow third party block developers, plugin developers, and core developers to access a more advanced component system for new features/plugins than is currently available to them without compromises. That means for new features in Gutenberg (like Global Styles) we can write it directly using the new components without needing to rely on any old APIs at all. This is also true for plugin developers and anyone else consuming the |
Replaced with #30503 where the discussion further continues. |
Background
#27331
#28399
Current approach
The current approach for integrating the new component system is as follows:
packages/components/src/ui
and refactor/rename things to match the WordPress conventions.@wordpress/components
, we write an adapter for it https://github.com/WordPress/gutenberg/blob/5cea9f8c34ee868203d135db14305a3a22644309/packages/components/src/text/next.js#L21ComponentSystemProvider
to activate the new component.This process and the thinking behind it is documented here: https://g2components.wordpress.com/2020/11/02/the-path-to-integration/
After having integrated several components, I think we need to revisit this strategy and whether it can support all our existing use cases.
The problem
This effectively "locks" all component system components behind existing WordPress components. This is fine right now as we're still working on community buy in. However, we're eventually going to run into a problem with being able to access the new components (for example how would you use
Heading
today? You cannot). Likewise there are two ways a WordPress component is used today:ComponentSystemProvider
and activate theWPComponentsButton
context, this "sort of" work but break halfway due to issues like the one I'm describing here.The second type of usage is a big problem for us moving forward because it means that for us to move into a world where Gutenberg is rendered using the new component system CSS-in-JS, we'll need to replace parts of Gutenberg piecemeal. This is going to be a lot of work. It's not just as simple as wrapping things in a provider and calling it a day.
Finally, by locking components behind existing APIs through the adapter, we'll end up keeping around the old APIs which we want to deprecate (especially in the case of components where the ergonomics of the APIs could be vastly improved, like Popover). But even if we deprecate them, there's no clear way for me to imagine right now that we'll be able to use the new API without the wrapper. For example, we'll never be able to rename the existing
Button
component toDeprecatedButton
or anything like that, and there's no sensible name for the newButton
other thanButton
. This is also a big problem for components like thePopover
where we want to fully deprecate the old API and there's no sensible way to write an adapter for it.The solution
So there's two problems here really, one is that we're not able to access the components from the new component system directly and the other related problem, is that the adapter strategy locks the new components behind deprecated APIs and, due to naming conflicts, potentially means we'll never be able to access certain new components directly.
One solution to this is that we could start to export the new components from
@wordpress/components/index.js
with the__experimental
prefix. This is complicated by the fact that some components already exist with that prefix, likeText
, and have adapters.Therefore, I think a better solution is to have a wholly separate package called
ui-components
for the new components to live in. The components would be exported from there and adapters could still be written for them in the oldcomponents
package. Then, as we replace the usages of certain components in Gutenberg, we can being to deprecate thecomponents
package piece meal. For example, if we replaced all usages of theButton
component in Gutenberg with the new component, we could add a deprecation message to the oldButton
and call it a day, knowing that the old API never needs to be supported again (though it will inevitably exist for WordPress's backwards compatibility guarantees).This doesn't remove the tremendous amount of work that we'll need to do to replace parts of Gutenberg piece meal but it makes the deprecation strategy cleaner and the adapter can stick around to make sure that the places that can take advantage of it, like certain blocks, do.
The new
ui-components
package can be complimented byui-styles
andui-create-styles
, the core of the style system.context
can be wrapped directly intoui-components
, I can't see a reason to expose it today or ever (it is extremely specific to the new component system).We don't necessarily have to publish these packages right away, but I think we should eventually aim towards publishing them as the new style system isn't something that just Gutenberg could benefit from. Indeed it is a general purpose style system that could be consumed by any React application.
In short
In short I think we should finish the current approach with the components listed in the tracking issue currently, but then, after we have community buy in and all that jazz is sorted out, we should move the new component system a new
ui-components
package along with new packages forui-create-styles
andui-styles
.This will create a simple and coherent separation between the to-be-deprecated components and the new components.
I'm curious what folks think about this. Is this a solution that people could be amenable to?
/cc @gziolo @ItsJonQ @youknowriad
The text was updated successfully, but these errors were encountered: