Element: enable concurrent mode by implementing mount/unmount with createRoot #46467
Conversation
|
Size Change: -173 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
packages/element/src/react-root.js
Outdated
| * @param {import('./react').WPElement} element Element to render. | ||
| * @param {HTMLElement} target DOM node into which element should be rendered. | ||
| */ | ||
| export function render( element, target ) { |
There was a problem hiding this comment.
As much as I'd like to just switch to the React 18 APIs and call it "done", I think we should play more responsibly towards the community here and keep exposing the legacy React 17 APIs. They might be necessary for consumers of the @wordpress packages that will upgrade to the React 18 dependency but will need additional time and effort to switch to the concurrent rendering mode.
My personal opinion is that we could, and should be exposing those legacy APIs indefinitely, or at least until we upgrade to a React version that stops supporting them or completely drops them.
There was a problem hiding this comment.
The main reason why I decided to reimplement render in terms of createRoot, instead of exporting createRoot from @wordpress/element, and use createRoot to mount the editor, is that createRoot is not very compatible with how we are currently mounting the editors. Let me explain.
Our editor packages, edit-post, edit-site, edit-widgets, export two functions like:
function initializeEditor( target ) {
render( <Editor />, target );
}
function reinitializeEditor( target ) {
unmountComponentAtNode( target );
render( <Editor />, target );
}This is easy to implement with the legacy render API, but what about createRoot? I would need to keep a Map from targets to roots, and then the two function would need to do lookups in that Map to convert the target argument to the root that corresponds to it.
Not a big deal, currently the render implementation in @wordpress/element does that internally, too, but for the initial version of the PR I decided to avoid this.
There was a problem hiding this comment.
That makes sense.
I think we can still keep your implementation, I actually like your idea about using a WeakMap here - however, my main concern is about backward compatibility and continuing to expose the old APIs the same way we were exposing them before.
So perhaps we can just continue exposing the legacy APIs, and keep your implementation, but rename it to something else? We could and probably should expose the raw React 18 APIs as well, just in case someone wants to use them.
There was a problem hiding this comment.
I think reinitializeEditor is in general called inside initialize meaning we can easily pass the root. That said, even that behavior of "retrying" when errors happens is something I think we should reconsider, I never used it and I believe it's just useless complication
There was a problem hiding this comment.
reinitializeEditor is a public API though. There's a wp.editPost.reinitializeEditor function exposed that anyone can call.
There was a problem hiding this comment.
mmm right, looking at the results here https://wpdirectory.net/search/01GM5BGZXFN1H5G6T568MC7C49 It seems no one is actually using it, so we could consider removing it (dev note). If we want to be extra careful we can use the weak map technique
There was a problem hiding this comment.
We could also deprecate it if we want to gracefully remove it.
|
There is trouble with mobile (RN) unit tests. The RN environment uses a minimalistic DOM implementation, And then crashes when trying to use it as object.
One solution would be to reexport the original I think it will be also worth reporting a React bug -- take into account that weird libraries like |
I definitely don't object to that 😉 Do you envision any downsides if we go with that approach? |
I see only upsides 🙂 Starting with the premise that |
jsnajdr
force-pushed
the
enable/react-18-concurrent-mode
branch
from
19b4697 to
5f4b0c9
Compare
jsnajdr
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
packages/edit-post/src/index.js
Outdated
| @@ -185,15 +134,13 @@ export function initializeEditor( | |||
| window.addEventListener( 'dragover', ( e ) => e.preventDefault(), false ); | |||
| window.addEventListener( 'drop', ( e ) => e.preventDefault(), false ); | |||
|
|
|||
| render( | |||
| createRoot( target ).render( | |||
There was a problem hiding this comment.
Might be worth returning the root in initializeEditor as well. So that we can allow users to unmount the tree as unmountComponentAtNode is replaced.
There was a problem hiding this comment.
I've given this a fair amount of testing and noticed that many features don't work at all and simply are static. I can't toggle sections and switch between tabs, unless I trigger an external re-render (@jsnajdr was demoing similar issues in a call yesterday).
I've noticed that when I disable StrictMode, it starts working again. cc @youknowriad who enabled it back in #7202.
So I've pushed a commit that removes strict mode for the time being.
If we end up with this, we might want to audit the repo for obsolete / legacy practices and refactor away from those as soon as possible.
Did you try to figure out why exactly these features (section toggling, tab switching) don't work? |
Yes, I'm seeing double rendering which also triggers the double running of effects, which seems to be unexpected for Gutenberg. In many areas our toggling mechanisms work like "enable if it's disabled, disable if it's enabled", and not "enable regardless of whether it's disabled, disable regardless of whether it's disabled". This should be working out of the box, but it doesn't when things run twice. It just makes it very difficult to locate a problem and start diagnosing it because with strict mode enabled there are just too many things that aren't working well. Are you suggesting that we keep it enabled and start fixing them one by one?
I don't think it does. It will produce a bunch of warnings when some of the legacy APIs or practices are used, but I don't see anything that would specifically disable concurrent features.
I'm not sure I understand what you mean - could you elaborate? |
I mean, if absence of |
It would be a nice project to fix all these double effects, and update the affected components to be concurrent-mode compatible. By the way @youknowriad already updated many components and hooks in #32765, the first version of React 18 migration, closed without merging, they are summarized in this comment: #32765 (comment) I'm wondering, were any of the components that you see double-effecting now, were they already modified in #32765? It still bugs me that in that PR Riad did so many updates that we didn't have to do in #45235 at all. |
jsnajdr
force-pushed
the
enable/react-18-concurrent-mode
branch
2 times, most recently
from
19b5379 to
f0b536f
Compare
Some of these were for e2e tests and some others were due to actual testing (maybe Safari?) but I don't recall entirely, I'm sorry and if there are real issues, we'll discover them anyway. Maybe it's StrictMode (which I believe we should reintroduce at some point). |
jsnajdr
force-pushed
the
enable/react-18-concurrent-mode
branch
from
3d20cb2 to
aa9cd41
Compare
|
I did some final updates to the
@youknowriad @kevin940726 @tyxla please let me know what you think about this latest update. After we get an agreement on it, the PR can be immediately merged, just in time for WP 6.2. |
|
These changes make sense.
Any reason for the returned value? I guess it's fine if we need it but we can also omit it?
Can we remove that code entirely or maybe it's exposed? |
Any reason why we want to remove this functionality as well? I think this can still be implemented without a public The other parts LGTM 👍 |
@kevin940726 suggested it here: #46467 (comment) and it makes sense: if the function mounts some React UI, it should provide ability to unmount it. We don't use it anywhere so there's also a solid argument against it: let's not create future compatibility issues with something that's not even used. |
The only place where this is exposed is the import { ErrorBoundary } from '@wordpress/editor'This component has an optional That looks fine to be removed, doesn't it? |
Reinitializing the editor on error is just a random thing to try, throwing stuff at a wall and seeing if it sticks. It doesn't have a solid justification. |
Thinking out loud: a way to justify it could be avoiding page reloading to prevent the loss of any unsaved data. |
|
I do agree that there's no guarantee that reloading the editor will help recover from a failure, but I've seen reinitialize working well sometimes, and that could prevent data loss if the user didn't save or autosave prior to the failure. I think we shouldn't rush to remove that functionality. |
Yeah, let's just remove. It's harmless indeed.
At the time, we implement this, we didn't have local storage persistence which we do today, so that argument doesn't hold I think. |
|
Removed the |
This is good to hear - I recall a complaint about data loss a few months ago, but I can't find a related issue. I think @mcsf reported it somewhere, but I can't find the convo. What you're saying makes sense to me, so let's try it out. |
| @@ -180,28 +173,24 @@ function Editor( { | |||
| } | |||
|
|
|||
| return ( | |||
| <StrictMode> | |||
There was a problem hiding this comment.
We should keep in mind to open a PR enabling StrictMode as soon as we land this one.
There was a problem hiding this comment.
Strict mode enabler PR opened here: #47639
|
@youknowriad @tyxla @kevin940726 -- if all feedback is addressed, could someone approve this so that I can merge? |
Reimplement the
render,hydrateandunmountComponentAtNodehelpers in@wordpress/elementusing the newcreateRootandhydrateRootAPIs from React 18, instead of using the legacy React 17 implementations.The side effect of this is switching the entire production editor to concurrent mode. Let's see if this breaks the editor or not.
In his original React 18 upgrade PR, @youknowriad did many changes to various components (summarized here) which were not included in the React 18 upgrade that got eventually merged. Maybe we'll need to merge these changes, too, after all, because otherwise concurrent mode will break? Let's test this hypothesis 🙂