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

Fixed destroy process of a single editor instance working within Context #356

Merged
merged 3 commits into from Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 29 additions & 10 deletions src/ckeditor.jsx
Expand Up @@ -145,6 +145,7 @@ export default class CKEditor extends React.Component {
async _initializeEditor() {
await this.editorDestructionInProgress;

/* istanbul ignore next */
if ( this.watchdog ) {
return;
}
Comment on lines +148 to 151
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this condition is no longer necessary. It was added while we were working on double rendering issue. But I leave it just in case.

Expand Down Expand Up @@ -230,16 +231,23 @@ export default class CKEditor extends React.Component {
async _destroyEditor() {
this.editorDestructionInProgress = new Promise( resolve => {
// It may happen during the tests that the watchdog instance is not assigned before destroying itself. See: #197.
//
// Additionally, we need to find a way to detect if the whole context has been destroyed. As `componentWillUnmount()`
// could be fired by <CKEditorContext /> and <CKEditor /> at the same time, this `setTimeout()` makes sure
// that <CKEditorContext /> component will be destroyed first, so during the code execution
// the `ContextWatchdog#state` would have a correct value. See `EditorWatchdogAdapter#destroy()` for more information.
/* istanbul ignore next */
if ( this.watchdog ) {
this.watchdog.destroy().then( () => {
this.watchdog = null;

setTimeout( () => {
if ( this.watchdog ) {
this.watchdog.destroy().then( () => {
this.watchdog = null;

resolve();
} );
} else {
resolve();
} );
} else {
resolve();
}
}
} );
Comment on lines +240 to +250
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea how to get rid of this setTimeout(), as the <CKEditor /> does not know if <CKEditorContex /> is going to destroy the editor.

Let's keep it as it is.

} );
}

Expand Down Expand Up @@ -348,9 +356,20 @@ class EditorWatchdogAdapter {
}

destroy() {
// Destroying an editor instance is handled in the `ContextWatchdog` class.
// Destroying an editor instance after destroying the Context is handled in the `ContextWatchdog` class.
// As `EditorWatchdogAdapter` is an adapter, we should not destroy the editor manually.
// Otherwise, it causes that the editor is destroyed twice.
// Otherwise, it causes that the editor is destroyed twice. However, there is a case, when the editor
// needs to be removed from the context, without destroying the context itself. We may assume the following
// relations with `ContextWatchdog#state`:
//
// a) `ContextWatchdog#state` === 'ready' - context is not destroyed; it's safe to destroy the editor manually.
// b) `ContextWatchdog#state` === 'destroyed' - context is destroyed; let `ContextWatchdog` handle the whole process.
//
// See #354 for more information.
if ( this._contextWatchdog.state === 'ready' ) {
return this._contextWatchdog.remove( this._id );
Copy link
Contributor

Choose a reason for hiding this comment

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

The this._contextWatchdog.remove() returns a promise, as expected.

}

return Promise.resolve();
}

Expand Down
89 changes: 89 additions & 0 deletions tests/issues/354-destroy-editor-inside-context.jsx
@@ -0,0 +1,89 @@
/**
* @license Copyright (c) 2003-2022, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* global window */

import React from 'react';
import { configure, mount } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import CKEditor from '../../src/ckeditor.jsx';
import CKEditorContext from '../../src/ckeditorcontext.jsx';

const { Context } = window.CKEditor5.core;

class CustomContext extends Context {}

configure( { adapter: new Adapter() } );

class App extends React.Component {
constructor( props ) {
super( props );

this.state = {
isLayoutReady: false,
renderEditor: true
};

this.editor = null;
}

componentDidMount() {
this.setState( { isLayoutReady: true } );
}

render() {
return (
<div className="row row-editor">
{ this.state.isLayoutReady && (
<CKEditorContext
config={ {} }
context={ CustomContext }
>
{ this.state.renderEditor && (
<CKEditor
onReady={ () => this.props.onReady() }
onChange={ ( event, editor ) => console.log( { event, editor } ) }
editor={ window.ClassicEditor }
config={ {} }
data={ '<p>Paragraph</p>' }
/>
) }
</CKEditorContext>
) }
</div>
);
}
}

describe( 'issue #354: unable to destroy the editor within a context', () => {
let wrapper;

beforeEach( () => {
return new Promise( resolve => {
wrapper = mount( <App onReady={ resolve }/> );
} );
} );

afterEach( () => {
if ( wrapper ) {
wrapper.unmount();
}
} );

it( 'should destroy the editor within a context', async () => {
wrapper.find( App ).setState( { renderEditor: false } );

await wait( 0 );

expect( wrapper.find( CKEditor ).exists() ).to.equal( false );
expect( wrapper.find( App ).getDOMNode().querySelector( '.ck-editor' ) ).to.equal( null );
} );
} );

function wait( ms ) {
return new Promise( resolve => {
setTimeout( resolve, ms );
} );
}