-
Notifications
You must be signed in to change notification settings - Fork 101
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ export default class CKEditor extends React.Component { | |
async _initializeEditor() { | ||
await this.editorDestructionInProgress; | ||
|
||
/* istanbul ignore next */ | ||
if ( this.watchdog ) { | ||
return; | ||
} | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea how to get rid of this Let's keep it as it is. |
||
} ); | ||
} | ||
|
||
|
@@ -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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
} | ||
|
||
return Promise.resolve(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 ); | ||
} ); | ||
} |
There was a problem hiding this comment.
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.