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

Added support for React.version in the @wordpress/element package #49312

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

dgwyer
Copy link
Contributor

@dgwyer dgwyer commented Mar 23, 2023

What?

Fixes #49308.

Include version as part of @wordpress/element.

Why?

Currently, you have to reference this from React directly.

import { version } from "react";

Now that WordPress 6.2 uses React 18, it can be necessary to check against the current version of React to avoid compatibility issues. See the associated issue: #49308

So this code:

import { version } from "react";
import { createRoot, render } from "@wordpress/element";
import App from "./app";

const domNode = document.getElementById("root");

if (parseInt(version) < 18) {
	render(<App />, domNode);
} else {
	const root = createRoot(domNode);
	root.render(<App />);
}

becomes

import { createRoot, render, version } from "@wordpress/element";
import App from "./app";

const domNode = document.getElementById("root");

if (parseInt(version) < 18) {
	render(<App />, domNode);
} else {
	const root = createRoot(domNode);
	root.render(<App />);
}

How?

React.version is imported from React and included in the exported items in @wordpress/element.

Testing Instructions

Go to any page and in the console enter: wp.element.version.

Screenshots or screencast

image

@margolisj
Copy link
Contributor

I almost feel like adding a version number to this package is a bandaid for a larger problem of gutenberg + wpjs package management throughout the different wordpress environments / sections / tools. Also, to me it would make sense to use wp.element.render to make the check for react.render or react.createRoot, otherwise what was the point of shadowing the react functions in the first place?

@aurooba
Copy link
Member

aurooba commented Mar 23, 2023

@margolisj While the initial issue deals with the render issue, it's not just that. There are other gotchas with React 18 as we gradually increase support in the ecosystem and for a while we will have to support React 17 and 18 for folks not on WordPress 6.2. So some kind of version check within element would be really helpful.

BUT you're also not wrong that it'd be nice if wp.element.render did the render related check for you, that would be in line with the philosophy behind having @wordpress/element in the first place.

@dgwyer
Copy link
Contributor Author

dgwyer commented Mar 23, 2023

As well as the reasons already outlined I just like the idea of having easy access to the current version of React on the WordPress site I'm working on. So, being able to just open a console window and enter wp.element.version, or do a check via React code, I can see being pretty useful in the future as more versions of React are released.

Also, I find it a little disorienting that there's no super simple for users to check the version of React via the Gutenberg editor UI. I'd personally find it useful to have this displayed somewhere. Not sure where it would be best to add this but here's what it could look like in the Preferences modal.

image

/**
* Current version of React.
*/
export { version };
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider calling it reactVersion or ReactVersion. Mere version suggests that it's the version of @wordpress/element, which it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qualifying it makes sense to me.

@aurooba
Copy link
Member

aurooba commented Mar 23, 2023

I don't know how I feel about the version being added to the UI. 1) that's not really in scope for this particular PR, and 2) I don't actually think that makes sense for the UI, maybe only if SCRIPT_DEBUG was turned on or something. That's really more dev-focussed.

@jsnajdr
Copy link
Member

jsnajdr commented Mar 23, 2023

BUT you're also not wrong that it'd be nice if wp.element.render did the render related check for you, that would be in line with the philosophy behind having @wordpress/element in the first place.

We discussed this back when working on the React 18 upgrade, and decided in favor of simply re-exporting the raw React APIs. You can read the discussion between #46467 (comment) and #46467 (comment).

Implementing the compatible render is not that easy. To also make unmounting work, we'd need to keep an internal map between elements and roots. Also, there are subtle differences between render and createRoot. render supports a callback argument to be called after rendering, createRoot doesn't support it. createRoot has some options that the old render doesn't have. What to do with them?

@dgwyer
Copy link
Contributor Author

dgwyer commented Mar 23, 2023

that's not really in scope for this particular PR

Just a general comment, not intended for this PR.

@aurooba
Copy link
Member

aurooba commented Mar 23, 2023

BUT you're also not wrong that it'd be nice if wp.element.render did the render related check for you, that would be in line with the philosophy behind having @wordpress/element in the first place.

We discussed this back when working on the React 18 upgrade, and decided in favor of simply re-exporting the raw React APIs. You can read the discussion between #46467 (comment) and #46467 (comment).

Implementing the compatible render is not that easy. To also make unmounting work, we'd need to keep an internal map between elements and roots. Also, there are subtle differences between render and createRoot. render supports a callback argument to be called after rendering, createRoot doesn't support it. createRoot has some options that the old render doesn't have. What to do with them?

Very good points! And I totally agree, upon further reflection. I know there's usually good discussions around this, thanks for linking to them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Including version in @wordpress/element
4 participants