Closed Mati365 closed 5 months ago
Totals | |
---|---|
Change from base Build d747c632-24f1-4d68-8f5a-d567b67a0b37: | 0.0% |
Covered Lines: | 70 |
Relevant Lines: | 70 |
@scofalik / @DawidKossowski No regressions according to QA testing results.
Do not force rebase, this branch will be used by new installation methods.
Note after call: It'll be merged after releasing new installation methods.
PR should now work with new installation methods
@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.
@DawidKossowski gotcha, will be removed
I will merge it after releasing NIM.
BTW, React 19 is delayed.
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:
May fix some of these bugs that seems to be related to Context:
ContextWatchdogContext
anduseCKEditorWatchdogContext
so now it is possible to implement own version of context used byCKEditor
)New features
useCKEditorWatchdogContext
ContextWatchdogValue
typings and guards (isContextWatchdogValue
,isContextWatchdogValueWithStatus
).ContextWatchdogContext
.❓ 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
New installation methods
All demos have been moved to new installation methods and use
ckeditor5
dependency.CKEditorContext
crashesCKEditorContext
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 ofCKEditorContext
in Strict Mode, resulting in the first watchdog instance being used in the childrenCKEditor
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.Child
CKEditorContext
components, such asCKEditor
component, incorrectly implemented detection of context state. After mounting of child, it was unknown if child was placed inCKEditorContext
or not becauseCKEditorContext
default value (while being initialized in async lifecycle methods) wasnull
. I addedinitializing
state to check if child is being initialized inCKEditorContext
.⚠️ Possible breaking changes
CKEditorContext
is no longer a class component, so these statements (which were highly error-prone and undocumented in our documentation) are no longer functional:status
field has been added to the context type, indicating when the watchdog has been fully initialized. CurrentContext
format: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
crashesIt 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.