ckeditor
React 19 support
Suggested merge commit message (convention)
Add support to React 19 and create demos. Adjust context format to prevent race conditions in CKEditor component.
Additional Information
Collaboration samples update PR: https://github.com/cksource/ckeditor5-collaboration-samples/pull/146 Closes https://github.com/ckeditor/ckeditor5-react/issues/478 https://github.com/ckeditor/ckeditor5-react/issues/312
May fix some of these bugs that seems to be related to Context:
- https://github.com/ckeditor/ckeditor5-react/issues/476 (race condition)
- https://github.com/ckeditor/ckeditor5-react/issues/294 (race condition)
- https://github.com/ckeditor/ckeditor5-react/issues/409 (exported
ContextWatchdogContextanduseCKEditorWatchdogContextso now it is possible to implement own version of context used byCKEditor)
New features
- Export context access hook
useCKEditorWatchdogContext
const Component = () => {
const context = useCKEditorWatchdogContext();
// ...
};
- Export context
ContextWatchdogValuetypings and guards (isContextWatchdogValue,isContextWatchdogValueWithStatus).
export type ContextWatchdogValue =
| {
status: 'initializing';
}
| {
status: 'initialized';
watchdog: ContextWatchdog;
}
| {
status: 'error';
error: ErrorDetails;
};
- Export context provider
ContextWatchdogContext.
const Component = () => {
// ...
return (
<ContextWatchdogContext.Provider value={yourCustomContext}>
..
</ContextWatchdogContext.Provider>
);
};
❓ Exports above allow slightly better customization of contexts used by CKEditor component. Especially mentioned https://github.com/ckeditor/ckeditor5-react/issues/409 in this thread.
Long story short
CKEditorContext crashes
CKEditorContext was using a class approach to define the component. However, with React 19, there have been changes in the calling order of lifecycle methods, which introduced race conditions in watchdog context creation that were fully reproducible in our demos. You can find an example here.
This error occurred because CKEditorContext created two instances of CKEditorContext in Strict Mode, resulting in the first watchdog instance being used in the children CKEditor component instead of the second one.
It was difficult to store additional information between the renders in a class component (constructor is called two times in strict mode), so I decided to refactor the component to a functional type. Unlike class components, functional components can hold some data between strict mode re-renders. Now, we store the last watchdog ID in a useRef and then check if the fully initialized watchdog ID matches the last requested watchdog ID. If it does, it is set to the state; otherwise, it is destroyed.
⚠️ Possible breaking changes
- The
CKEditorContextis no longer a class component, so these statements (which were highly error-prone and undocumented in our documentation) are no longer functional:
const contextRef = useRef();
useEffect( () => {
// Right now it is not possible to access `contextWatchdog` anymore and it will return `undefined`.
const { contextWatchdog } = contextRef.current;
// ...
}, [] );
<CKEditorContext
...
ref={ contextRef }
/>
- The
statusfield has been added to the context type, indicating when the watchdog has been fully initialized. CurrentContextformat:
export type ContextWatchdogValue =
| {
status: 'initializing';
}
| {
status: 'initialized';
watchdog: ContextWatchdog;
}
| {
status: 'error';
error: ErrorDetails;
};
It simplifies deduction and prevents race condition in reusing watchdog logic inside components. Especially here. From now, the CKEditor component is waiting for fully initialization of watchdog to start bootstrapping editor.
However, CKEditorContext (React Context) was not exported from the package, so it should not be used by external integrations. I would mark it as a potential breaking change because there is one additional re-render due to the change from initializing to initialized state set in the context.
useMultiRootEditor crashes
It was impossible to register new root in React 19 due to this exception. It happened because EditorEditable was rendered two times in strict mode and second execution was performed on the DOM element that was already assigned to editor.
Pull Request Test Coverage Report for Build 2bc51473-7e69-462a-a7b2-65b22bfb53b7
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 100.0%
| Totals | |
|---|---|
| Change from base Build d747c632-24f1-4d68-8f5a-d567b67a0b37: | 0.0% |
| Covered Lines: | 70 |
| Relevant Lines: | 70 |
💛 - Coveralls
@f1ames Can you take a look at this PR? I've heard that you worked with React before, it'd be nice to hear additional feedback about changes. Thanks!
@DawidKossowski
Unit tests produce a lot of errors in the console
It happens when CKEditor has roots that are not assigned to DOM while being destroyed. Happens frequently in renderHook test suite. I added fake elements before destroying editor to prevent this situation (commit). Overall I think it's not super crucial and maybe useful in testing only.
I got this error once in the Context demo (React 19):
I cannot reproduce this race condition altough I believe that you are right. There can be race condition between synchronization of watchdog state and React state. Basically context might be destroyed and react state update (about reinitialization of such react context) is still pending while editor is initializing. I pushed commit that fixes the issue I think.
The isLayoutReady prop in CKEditorContext works differently in the "classic" integration and multi-root integration. After providing isLayoutReady={false} in demo-multiroot-react-19, roots are rendered, and you can edit the content. However, when you pass the same prop in demo-react-19, the editor is not shown.
Is it correct behavior? It looks like our current implementation is broken (or it has another race condition). It makes absolutely no sense to initialize any editor within ContextProvider without waiting for initialization of context (because it will fall into the context adapter fallback in these lines).
Generally, I understand it was a good opportunity to refactor some code, but adding new functionalities made the overall changes unclear. I'm not sure which changes were necessary in the context of supporting React 19 and which were made for refactoring purposes. I believe we could have done it in two steps. Additionally, I need to rethink the additional types, guards, and so on.
Generally speaking it's complicated because it solves complex problem. React 19 tends to be more async than any other previous release and it only shown us hidden for many years issues with our implementation. The example might be async lifecycle methods in context class component which were reported already by community. These methods added other smaller race conditions. StrictMode another ones. null value in default context value too. This PR fixes all these smaller things to fix "big" Race Condition issue.
I believe we could have done it in two steps.
Agree however this PR is the first step. The next ones should be related to DX improvements and should utilize newer react hooks such as https://react.dev/reference/react/useSyncExternalStore which might help us with fixing with that kind of race condition issues (I hope so at least).
Probably fixes this issue https://github.com/ckeditor/ckeditor5-react/issues/485#issue-2343726122. Must be checked.
Probably fixes this issue #485 (comment). Must be checked.
Shouldn't those fixes be made against master/alpha to release a new version?
@Witoso At this moment, nope. Too many changes. Initially, I thought that the watchdog race conditions were only reproducible on React 19. However, it turns out that they can also occur occasionally on older versions. On React 19, these race conditions happen consistently.
Hmm, will it mean that context in next.js won't work until we release this PR?
@Witoso I suspect that it's reproducible on old installation methods, actually. Needs to be verified.
@Witoso I suspect that it's reproducible on old installation methods, actually. Needs to be verified.
Yes, I checked with old installation methods and stable ckeditor5-react package and there is the same issue as reported in #485 for NIM setup.
Generally speaking it's complicated because it solves complex problem.
@Mati365, sure, the problem is complex, and it is understandable. However, you have exported a lot of new things as a public API. Who should use it and when? We need to provide user guides describing the correct use cases and so on.
Moreover, we still want to refactor the whole integration. In that case, the public API could change. I would rather reduce the breaking changes in the future by only making internal changes that fix the issue instead of extending the API. That is what I meant.
@DawidKossowski I thought it may be useful here. I don't think that context format will be changed in future iterations so much. If you think we should not export it, I'll remove it.
That's what I meant by doing it in two steps. The first step will fix the integration with React 19 internally (the current PR), and the second step will enhance DX, the public API, and so on. I think the second step should be done as a separate task since it requires some research, analysis, and probably discussion. I'm not 100% sure about the current shape, and I don't want to block this PR, so I would remove it and address it in another task.