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

Reconsider @wordpress/element as a React wrapper and use React more directly. #54074

Closed
youknowriad opened this issue Aug 30, 2023 · 14 comments
Closed
Labels
[Type] Enhancement A suggestion for improvement.

Comments

@youknowriad
Copy link
Contributor

When initially evaluating frameworks, we built the @wordpress/element package as a wrapper around React to potentially support more framework or account for breaking changes. I think it's time for us to evaluate that decisions.

Most of our packages that are React libraries (compose, components...) do the following today:

  • Define React as a peer dependency
  • Ship files that use createElement from @wordpress/element instead of React

I believe that this is a contradiction and is not great for multiple reasons:

  • It's not clear whether our packages should be considered "React" libraries or "@wordpress/element" libraries.
  • It's not really clear what the role of @wordpress/element is is anymore because we expose almost all the APIs of React through the package without any change or adaptation.

Note that we do have some functions in @wordpress/element that are additional like renderToString or createInterpolateElement...

Also let's note that any potential breaking change that happens in React in the future will be automatically a breaking change in WordPress as well because "React" is also a public API in WordPress through the "react" script handle. In other words, wordpress/element's abstraction doesn't allow us to prevent future potential React breaking changes.

Proposal

For these reasons, I'd like to propose that we simplify things a little bit (which will also make it easier for third-party developpers to consume our packages):

  • I'd like for our packages to become simple "React libraries", in other words they'd ship React.createElement instead of createElement from @wordpress/element
  • Keep the peer dependencies as they are (aka React)
  • If a package needs an extra function that is provided by @wordpress/element they can import it from there like any regular library.
  • remove any special JSX bundling config we have (just rely on the community tools for JSX)
  • In other words, we stop pretending like @wordpress/element is a wrapper around React.

Just noting as well that such a change won't have any impact on WordPress. It won't introduce any breaking changes to WordPress. It will introduce what can be seen as a breaking change for the wp-scripts dependency as it will switch from generating wp.element.createElement to React.createElement... but since it's a dev tool that is upgraded manually by developers, this is acceptable.

The pros of this change is that it will make consuming @WordPress packages through npm way simpler for any third-party JS application.

cc @WordPress/gutenberg-core @jsnajdr @gziolo

@nerrad
Copy link
Contributor

nerrad commented Aug 30, 2023

Just noting as well that such a change won't have any impact on WordPress. It won't introduce any breaking changes to WordPress. It will introduce what can be seen as a breaking change for the wp-scripts dependency as it will switch from generating wp.element.createElement to React.createElement... but since it's a dev tool that is upgraded manually by developers, this is acceptable

While generally, I'm supportive of this change, I think we have to be careful about stating this won't have any impact on WordPress (although I think I understand what you meant by this). There will be impact on:

  • documentation
  • examples or actual code in the wild that isn't created by any build process and directly refers to wp.element.createElement (or other wrapped functions).
  • the long tail of tutorials etc that promote/recommend using wp.element instead of React.

This can all be mitigated to a degree but I think it's still worth acknowledging that impact.

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 30, 2023

@nerrad Good point about the documentation changes / tutorials etc... Just noting that regardless of whether they compile to wp.element or React. Scripts will continue to work for the foreseeable future.

@noahtallen
Copy link
Member

noahtallen commented Aug 30, 2023

I agree with this change too. I'm not 100% aware of all of the original arguments, but at this point, if the project were to switch to a different library (which is unlikely IMO), it'd still require large breaking changes to @wordpress/element.

For the sake of argument, say we switched to a different library. We'd still need to keep React support for some time for backwards compatibility. As a result, the new framework functions would need to be under a different namespace in @wordpress/element or in a different package. What if this other framework also has createElement that works differently? We can't simply replace our function without breaking existing React code, and it'd be unlikely for WordPress to immediately drop React in a version without a deprecation period.

So it'd need to be exposed under a different name (and probably a different package or namespace within @wordpress/element for clarity) anyways. If that's the case, why not just use the framework package directly?

So I agree that @wordpress/element is mostly just a layer of indirection that doesn't help us much.

@Mamaduka
Copy link
Member

We'll still have to maintain the @wordpress/element package for the foreseeable future.

The way I see it, the pro of simple package consumption doesn't really benefit the WP while introducing more work for maintainers.

P.S. I generally agree with the proposal; we just need to be sure that the pros outweigh the cons.

@youknowriad
Copy link
Contributor Author

The way I see it, the pro of simple package consumption doesn't really benefit the WP while introducing more work for maintainers.

I'm not sure I understand what you mean when you say "more work". Can you clarify?

@tyxla
Copy link
Member

tyxla commented Sep 6, 2023

Thanks for the proposal @youknowriad! I agree with it, and I must say this is something we've already been considering for various reasons, including:

  • In the present day, @wordpress/element does nothing on top of being a wrapper around react and react-dom, thus is unnecessary. There was a time when we weren't sure about the underlying library choice, or whether we'll need to patch any of the functions, but that is now in the past.
  • It requires bundling both react and react-dom together, even if only one of them is used. That way, even if react-dom is not necessary, it's still bundled if @wordpress/element is used. That substantially inflates some of the packages, and by using the libraries directly, some packages and external projects that use those packages will immediately benefit from bundle size improvements.
  • Packages that utilize @wordpress packages historically have needed to update both @wordpress packages and the underlying react/react-dom (and a few more) packages together, which has historically made the update tricky because of all the peer dependency issues. By unbundling react and react-dom those updates will now be easier.

There is the downside of having to maintain the element package to maintain backward compatibility, but we can totally deprecate the package and even stop maintaining it. By using an old react / react-dom version its external consumers will naturally migrate away from it as they start updating to newer react versions in the future.

I realize that removing @wordpress/element may be a time-taking process, but I think it's worth it and I'm personally happy to devote time to such a migration.

@Mamaduka
Copy link
Member

Sorry for the late reply, @youknowriad 🙇

I'm not sure I understand what you mean when you say "more work".

Maintaining the package for BC, updating documentation, and swapping imports in the core.

@fabiankaegy
Copy link
Member

The longer I think about this the more I think it's a bad idea but throwing it out there anyways.

Would it make sense to use the DependencyExtractionWebpackPlugin to automatically add a mapping of @wordpress/element to react for anyone within the context of WordPress? 🤔

I know that I for example have shipped a lot of usages of import { useState } from @wordpress/element` in my time over the last several years. All of them use a build process and so having the remapping happen automatically would be much nicer than having to eventually update them all...

Also when in the context of WordPress I personally quite like the mental modal of @wordpress/* dependencies meaning they are not bundled in my scrip but instead are loaded from WordPress itself as a dependency.

I know this already isn't the case and things like lodash and even react already break that rule because they also get handled by WordPress directly. And conversely @wordpress/icons or @wordpress/interface are not available that way.

But it does seem more "strange" to import something that you don't have installed on the project that isn't namespace as a WordPress dependency.

I very much understand the benefits for the project as a whole though :)

@sethrubenstein
Copy link
Contributor

Can I ask what is the reason for this change, what is gained by doing this?

We exclusively reference @wordpress/element not React in our work because thats what it is. You can't take our components and pop them into any React app because they're not meant for that, they're meant to be run under a WordPress React environment driven by @wordpress/element.

@juanmaguitar
Copy link
Contributor

Does this change mean that methods imported from @wordpress/element will need to be imported from react after this change?

This is, these calls...

import { forwardRef } from '@wordpress/element';
import { useEffect, useRef } from '@wordpress/element';
import { useState, useEffect } from '@wordpress/element';
import { Children, cloneElement, forwardRef } from '@wordpress/element';

will need to be transformed into the following?

import { forwardRef } from 'react';
import { useEffect, useRef } from 'react';
import { useState, useEffect } from 'react';
import { Children, cloneElement, forwardRef } from 'react';

Or will the @wordpress/element continue exporting React methods for compatibility reasons but internal transformation (wp.element.createElement to React.createElement) will be done using just React and @wordpress/element won't be recommended as a dependency anymore from the docs?

@mikeritter
Copy link

Abstraction is ALWAYS important when developing a framework like the new WordPress.

Coupling to a particular library (i.e., React) makes the framework dependent on changes in that ecosystem.

Where wordpress/element is dependent on React, THAT should be abstracted away, not more-tightly integrated.

@youknowriad
Copy link
Contributor Author

Thanks for the feedback @fabiankaegy @sethrubenstein @juanmaguitar and @mikeritter let me clarify a few things:

First let's make it clear, this change doesn't impact WordPress developers at all:

  • They can already use either the "react" script or "wp-element" scripts to build their plugin, and they can continue to do as they wish. For the moment, no deprecation message or anything will be shown to developers and I'm not sure we'll ever deprecate anything here.

Now, that this is out of the way, let me try to address your points above:

I know that I for example have shipped a lot of usages of import { useState } from @wordpress/element` in my time over the last several years. All of them use a build process and so having the remapping happen automatically would be much nicer than having to eventually update them all...

I don't think it makes sense to remap automatically, because the packages are not strictly equivalent, we expose most of React APIs in @wordpress/element but we also expose more APIs.

Also when in the context of WordPress I personally quite like the mental modal of @wordpress/* dependencies meaning they are not bundled in my scrip but instead are loaded from WordPress itself as a dependency.

I understand but also as you said, wordpress ships "react", "lodash", "tinymce", "moment"... so that point don't really hold globally IMO. you can also "import" a number of these dependencies when using wp-scripts for a long time now. There's no change.

Can I ask what is the reason for this change, what is gained by doing this?

The reason is explained above, it's not targeted for WordPress developers, it's targeted for users of the @wordpress/* packages in any application, whether it's a WordPress headless JS frontend or any JS app really. These developers usually rely on JSX being transformed in React in their build tools (it's the default for most build tools) and they use react libraries, so they expect that any react library work. So it's both a clarification and a DevX improvement for these users. You can read this issue #53874 to have an idea about this kind of users and their existing struggles.

Does this change mean that methods imported from @wordpress/element will need to be imported from react after this change?

No, no change is needed but IMO, we should recommend using "react" directly because both of them are exposed already and the abstraction doesn't really bring any value.

Abstraction is ALWAYS important when developing a framework like the new WordPress.

Oh I agree that abstraction is necessary in development but the reality as I explained above, is that this is not really an abstraction. It's what we call a leaky abstraction since you can access the underlying framework and use it already.

Also, a lot of people do use the underlying framework, if you've built a plugin using these scripts already, you might have used an external react library like "radix", "react-redux" or any React library really. Developers expect these libraries to work by default even if they're using wp.element.createElement. So what they're actually using is "react" already not an abstraction. In other words, any component written for "react" is expected to work in "@wordpress/element". So it's not really an abstraction, folks are using "React" directly, it's just a shortcut.


I hope this clarifies a few things.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 2, 2023

A very similar question about API design was recently raised by @luisherranz when working on the Interactivity API. The Interactivity API uses Preact under the hood, exposes its API to WordPress developers, and also adds its own custom APIs. Now the question is, should we import them all from one @wordpress/* package, like:

import { useEffect, useInit, useWatch } from "@wordpress/interactivity";

or to import the native Preact stuff from the preact package, and import only the WordPress extensions from the @wordpress one?

import { useEffect } from "preact";
import { useInit, useWatch } from "@wordpress/interactivity";

I think there are good reasons for both approaches, and the answer depends on who you are and what is your point of view.

You can be a "WordPress developer," with your view centered around the WordPress platform, and all things React are just little extensions of this platform. Then it's natural to have all the platform APIs named with the @wordpress/* prefix, because that's what they are, API for developing WordPress blocks or interactivity experiences.

But you can also be a "React developer," who works on React projects, has knowledge about React itself, and is trying to apply and reuse this knowledge on a WordPress project. React, just like WordPress, is also a huge ecosystem with a community, documentation, code examples, Stack Overflow questions, blog posts, third party libraries and tooling.

By trying to abstract React away and pretending everything is just @wordpress/*, you are kind of cutting yourself away from that ecosystem, and making it harder for you to benefit from it. It will be harder for React experts to answer your questions, because they will be confused by the @wordpress/element imports they are not familiar with. Code examples need to be adapted to the @wordpress/element conventions. ChatGPT will have a harder time generating code for you because it's trained on code that imports react, not on @wordpress/element. Tools like transpilers and linters will need to be specially configured for @wordpress/element, the defaults won't work. Looking up documentation on useEffect or useState will always return much better results for react than for @wordpress/element.

By importing directly from react, you're also sort of "telling the truth" -- yes, the underlying library is React, and even when hidden by the @wordpress/element facade, it will always be React. It's unrealistic to replace the implementation with something else that behaves the same. The library is too complex for that, with many subtle behaviors that would need to be exactly reproduced, like the exact order of calls to various callbacks (render functions, refs, effects, ...).

For these reasons, it makes sense to me to continue exporting React and Preact API under both naming conventions: react/preact and @wordpress/element/interactivity. Both have large audiences for which the given convention is ideal.

@youknowriad
Copy link
Contributor Author

The docs and wp-scripts package have both been updated. I think we can close this issue right now. We still have a lot of code in Gutenberg that uses @wordpress/element that could be using react directly but that seems fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

10 participants