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

onChangeCalled when ck-editor should have been removed #168

Closed
ansorensen opened this issue Jul 8, 2020 · 4 comments · Fixed by #178
Closed

onChangeCalled when ck-editor should have been removed #168

ansorensen opened this issue Jul 8, 2020 · 4 comments · Fixed by #178

Comments

@ansorensen
Copy link

📝 Provide detailed reproduction steps (if any)

In this sandbox, I create an array of ckeditor-react instances. The value of these editors can be changed. Or the entire value the editor represents can be removed from the array.

  1. Click the 'x' button by the second editor

✔️ Expected result

The 2nd line (editor with 'two' and its button) should be removed. Expect only the handle remove function to called.

❌ Actual result

There are still 3 editors, now display "one", "three", "three". You can see from the console logs that the new state should be ["one", "three"]. Instead, the onChange is fired and resolve after the removal, resulting in ["one", "<p>three</p>", "three"]

📃 Other details

  • Browser: chome
  • OS: mac
  • CKEditor version: 2.1.0
  • Installed CKEditor plugins: out of the box classic build (20.0.0)

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@ansorensen
Copy link
Author

ansorensen commented Jul 8, 2020

Note that there is a work around for setState calls from stale state(using a function in the set call, in order to perform an operation relative to current state). However, it would be even better to not have to worry about vendor libraries making stale state calls if we can figure out how to not trigger a changeEvent when the editor is not truly being changed It's inheriting new data value from the outside.

My actual use case is creating a wiget of that manages an array of data items that include ckeditor5-react as an input for rich text. My ideal interface would simply be:
<widget items={items} onChange={(newItems) => setItems(newItems)} />.

However, because I have to be careful about text changes, I have to implement a more complicated interface:
<widget item={items} onChange={(newItems) => setItems(newItems)} onTextChange={(textChangedItem, index) => setItems( (currentItems) => [...currentItems.slice(0, index), textChangedItem, ...currentItems.slice(index+1)])} />

Without worrying about stale change calls, all that onTextChange logic can actually be done in the widget, leaving one clean interface for updating the state of the parent's array. If I use this widget in multiple context, each context has to replicate that text change logic.

@jodator
Copy link

jodator commented Jul 10, 2020

You need to have a reference or a map of the created editors in order to call editor.destroy() on destroyed editor instance.

@Reinmar
Copy link
Member

Reinmar commented Aug 10, 2020

@jodator, I think you misunderstood the example given by @ansorensen. In the React integration, the editor is destroyed automatically when the component unmounts. You should not need to care about this.

Here, the problem, if I understand correctly, is that when you press the "X" button you get:

  • the state change: "remove 2nd editor"
  • but then, suddenly, the onChange callback of that editor is executed, even though this editor should only be destroyed at that point

So, you get a change event from a component that should actually not be any more available.

As @ansorensen wrote, it's possible to use a callback-based setFoo() form, yet, this seems to be broken in our component or even in CKEditor 5 itself. I checked cke5-react code and the onChange callback is only executed on change:data. I've got no idea why it's executed in this case.

I'm transferring this to the cke5-react repo.

@ansorensen, could you confirm that I understood the example correctly?

@Reinmar Reinmar transferred this issue from ckeditor/ckeditor5 Aug 10, 2020
@Reinmar Reinmar added this to the nice-to-have milestone Aug 10, 2020
@ansorensen
Copy link
Author

@Reinmar Yeah, that's accurate. @jodator Your comment might work, i.e. manually shutting down the editor itself directly before removing the React component... However, the fact that the React component cannot handle it's own removal without trying an onChange seems buggy. You would expect the work of destroying the editor instance to be reliably abstracted out for you by the React component.

pomek added a commit that referenced this issue Sep 18, 2020
Feature: The `<CKEditor>` component contains the built-in [watchdog](https://ckeditor.com/docs/ckeditor5/latest/features/watchdog.html) feature. Closes #118. 

Feature: Introduced the `<CKEditorContext>` component that supports the [context](https://ckeditor.com/docs/ckeditor5/latest/features/collaboration/context-and-collaboration-features.html) feature.

Feature: Added the `id` property which is used to distinguish different documents. When this property changes, the component restarts the underlying editor instead of setting data on it, which allows e.g. for switching between collaboration documents and fixes a couple of issues (e.g. the `onChange` event no longer fires during changing the document). Closes #168. Closes #169.

Feature: The `onError()` callback will be called with two arguments. The first one will be an error object (as it was before the release 3+). A second argument is an object that contains two properties:

  * `{String} phase`: `'initialization'|'runtime'` - Informs when the error has occurred (during the editor/context initialization or after the initialization).
  * `{Boolean} willEditorRestart` - When `true`, it means that the editor component will restart itself.
  * `{Boolean} willContextRestart`- When `true`, it means that the context component will restart itself.

    The `willEditorRestart` property will not appear when the error has occurred in the context feature. 
    The `willContextRestart` property will not appear when the error has occurred in the editor.

---

* _Add to the release summary:_ Both components (`<CKEditor>` and `<CKEditorContext>`) will internally use the [`Watchdog`](https://ckeditor.com/docs/ckeditor5/latest/api/module_watchdog_watchdog-Watchdog.html) class that restarts the [editor](https://ckeditor.com/docs/ckeditor5/latest/api/module_watchdog_editorwatchdog-EditorWatchdog.html) or [context](https://ckeditor.com/docs/ckeditor5/latest/api/module_watchdog_contextwatchdog-ContextWatchdog.html) when an error occurs.

* _Add to the release summary:_ The API of the entry point of the package has changed.

The previous way how the package was imported:

```js
import CKEditor from '@ckeditor/ckeditor5-react';
```

will not work with the release (3+). Use:

```js
import { CKEditor } from '@ckeditor/ckeditor5-react';
```

---

BREAKING CHANGE: The [entry point](https://github.com/ckeditor/ckeditor5-react/blob/master/src/index.js) of the package has changed. The default import was removed since the package provides more than a single component now. Use `import { CKEditor } from '@ckeditor/ckeditor5-react'` instead of `import CKEditor from '@ckeditor/ckeditor5-react';`.

BREAKING CHANGE: The `onInit` property was renamed to `onReady` and can be called multiple times (after the initialization and after the component is ready when an error occurred).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants