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

Add onAfterDestroy callback description to react docs #16281

Merged
merged 4 commits into from May 20, 2024

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Apr 24, 2024

Suggested merge commit message (convention)

Docs: Add onAfterDestroy callback description to React integration docs.

Why has been this callback introduced?

  1. Editor uses async destroy method that does not work super fine with React lifecycle.
  2. Editor is most of the time destroyed after unmounting of component or changing its ID property.
  3. This callback makes sure that all side effects (such as added editable HTML DOM nodes) are removed after destroying all plugins attached to the editor.
  4. onAfterDestroy is being batched, so in other words rapid id change (10 in row) will trigger only ~1 call of this callback which prevents race conditions in Strict Mode.

Additional information

  1. The onAfterDestroy callback has been added in this commit in our React integration.
  2. It's not released yet at this moment (20.05.2024).

@ckeditor ckeditor deleted a comment from coderabbitai bot May 20, 2024
@Mati365
Copy link
Member Author

Mati365 commented May 20, 2024

Thank you @godai78.

@pomek Can you merge it after release of React integration?

fyi @scofalik

@Mati365 Mati365 requested a review from Reinmar May 20, 2024 06:09
Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

Except for the mentioned things, please target the PR to the #stable branch if we aim to deploy these changes without waiting until the next release.

docs/installation/integrations/react.md Show resolved Hide resolved
docs/installation/integrations/react.md Outdated Show resolved Hide resolved
@Mati365
Copy link
Member Author

Mati365 commented May 20, 2024

@pomek It can be deployed in normal flow I think. There is no need to deploy it instantly after merge because this callback is not critical to work with integration (especially because it was not present for years). Thanks for suggestion to docs anyway.

@scofalik
Copy link
Contributor

LGTM, you can merge it. As far as I understand, neither the new callback nor the docs for the callback are going to be released in this release (which should happen very shortly)?

@pomek
Copy link
Member

pomek commented May 20, 2024

The required package has been released (https://github.com/ckeditor/ckeditor5-react/releases/tag/v7.0.0).

@pomek pomek merged commit 8e01ccf into master May 20, 2024
6 checks passed
@pomek pomek deleted the ck/add-after-destroy-to-react-docs branch May 20, 2024 11:17
@godai78
Copy link
Contributor

godai78 commented May 20, 2024

We have ported these changes to new installation docs, which we plan on merging to master sometime soon, so this will be wvisible on nightly.

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.

None yet

4 participants