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

Minor improvements for package's components and tests #229

Merged
merged 11 commits into from
Mar 26, 2021
9 changes: 6 additions & 3 deletions scripts/utils/getkarmaconfig.js
Expand Up @@ -70,11 +70,11 @@ module.exports = function getKarmaConfig() {
customLaunchers: {
CHROME_TRAVIS_CI: {
base: 'Chrome',
flags: [ '--no-sandbox', '--disable-background-timer-throttling' ]
flags: [ '--disable-background-timer-throttling', '--js-flags="--expose-gc"' ]
},
CHROME_LOCAL: {
base: 'Chrome',
flags: [ '--disable-background-timer-throttling' ]
flags: [ '--disable-background-timer-throttling', '--js-flags="--expose-gc"' ]
}
},

Expand Down Expand Up @@ -123,7 +123,10 @@ module.exports = function getKarmaConfig() {
webpackConfig.module.rules.push( {
test: /\.jsx?$/,
loader: 'istanbul-instrumenter-loader',
include: /src/,
include: [
/src/,
/_utils-tests/
],
exclude: [
/node_modules/
],
Expand Down
63 changes: 45 additions & 18 deletions src/ckeditor.jsx
Expand Up @@ -21,13 +21,15 @@ export default class CKEditor extends React.Component {
/**
* An instance of EditorWatchdog or an instance of EditorWatchdog-like adapter for ContextWatchdog.
*
* @type {EditorWatchdog|EditorWatchdogAdapter}
* @type {module:watchdog/watchdog~Watchdog|EditorWatchdogAdapter}
*/
this.watchdog = null;
}

/**
* An editor instance.
*
* @type {module:core/editor/editor~Editor|null}
*/
get editor() {
if ( !this.watchdog ) {
Expand All @@ -37,17 +39,20 @@ export default class CKEditor extends React.Component {
return this.watchdog.editor;
}

// The CKEditor component should not be updated by React itself.
// However, if the component identifier changes, the whole structure should be created once again.
/**
* The CKEditor component should not be updated by React itself.
* However, if the component identifier changes, the whole structure should be created once again.
*
* @param {Object} nextProps
* @return {Boolean}
*/
shouldComponentUpdate( nextProps ) {
if ( !this.editor ) {
return false;
}

// Only when the editor component changes the whole structure should be restarted.
// Only when the component identifier changes the whole structure should be re-created once again.
if ( nextProps.id !== this.props.id ) {
this._destroyEditor();

return true;
}

Expand All @@ -62,22 +67,33 @@ export default class CKEditor extends React.Component {
return false;
}

// Initialize the editor when the component is mounted.
/**
* Initialize the editor when the component is mounted.
*/
componentDidMount() {
this._initializeEditor();
}

// Initialize the editor when the component is updated as it should be destroyed before the update.
/**
* Re-render the entire component once again. The old editor will be destroyed and the new one will be created.
*/
componentDidUpdate() {
this._destroyEditor();
this._initializeEditor();
}

// Destroy the editor before unmouting the component.
/**
* Destroy the editor before unmounting the component.
*/
componentWillUnmount() {
this._destroyEditor();
}

// Render a <div> element which will be replaced by CKEditor.
/**
* Render a <div> element which will be replaced by CKEditor.
*
* @return {JSX.Element}
*/
render() {
return (
<div ref={ this.domContainer }></div>
Expand All @@ -86,12 +102,14 @@ export default class CKEditor extends React.Component {

/**
* Initializes the editor by creating a proper watchdog and initializing it with the editor's configuration.
*
* @private
*/
_initializeEditor() {
if ( this.context instanceof ContextWatchdog ) {
this.watchdog = new EditorWatchdogAdapter( this.context );
} else {
this.watchdog = new EditorWatchdog( this.editor );
this.watchdog = new CKEditor._EditorWatchdog( this.props.editor );
}

this.watchdog.setCreator( ( el, config ) => this._createEditor( el, config ) );
Expand All @@ -107,6 +125,7 @@ export default class CKEditor extends React.Component {
/**
* Creates an editor from the element and configuration.
*
* @private
* @param {HTMLElement} element The source element.
* @param {Object} config CKEditor 5 editor configuration.
* @returns {Promise}
Expand Down Expand Up @@ -157,6 +176,8 @@ export default class CKEditor extends React.Component {

/**
* Destroys the editor by destroying the watchdog.
*
* @private
*/
_destroyEditor() {
// It may happen during the tests that the watchdog instance is not assigned before destroying itself. See: #197.
Expand All @@ -172,7 +193,8 @@ export default class CKEditor extends React.Component {
/**
* Returns true when the editor should be updated.
*
* @param {*} nextProps React's properties.
* @private
* @param {Object} nextProps React's properties.
* @returns {Boolean}
*/
_shouldUpdateEditor( nextProps ) {
Expand All @@ -192,6 +214,12 @@ export default class CKEditor extends React.Component {
return true;
}

/**
* Returns the editor configuration.
*
* @private
* @return {Object}
*/
_getConfig() {
if ( this.props.data && this.props.config.initialData ) {
console.warn(
Expand Down Expand Up @@ -277,12 +305,7 @@ class EditorWatchdogAdapter {
* An editor instance.
*/
get editor() {
// TODO - try/catch should not be necessary as `getItem` could return `null` instead of throwing errors.
try {
return this._contextWatchdog.getItem( this._id );
} catch ( err ) {
return null;
}
return this._contextWatchdog.getItem( this._id );
}
}

Expand Down Expand Up @@ -313,3 +336,7 @@ CKEditor.defaultProps = {
config: {},
onError: ( error, details ) => console.error( error, details )
};

// Store the API in the static property to easily overwrite it in tests.
// Too bad dependency injection does not work in Webpack + ES 6 (const) + Babel.
CKEditor._EditorWatchdog = EditorWatchdog;
63 changes: 45 additions & 18 deletions src/ckeditorcontext.jsx
Expand Up @@ -13,34 +13,59 @@ export default class CKEditorContext extends React.Component {
constructor( props, context ) {
super( props, context );

/**
* @type {module:watchdog/contextwatchdog~ContextWatchdog|null}
*/
this.contextWatchdog = null;

if ( this.props.isLayoutReady ) {
this._initializeContextWatchdog( this.props.config );
this._initializeContextWatchdog();
}
}

/**
* Checks whether the component should be updated by React itself.
*
* @param {Object} nextProps
* @return {Boolean}
*/
shouldComponentUpdate( nextProps ) {
// If the configuration changes then the ContextWatchdog needs to be destroyed and recreated
// On top of the new configuration.
// If the config has changed then the `ContextWatchdog` needs to be destroyed and recreated using the new config.
if ( nextProps.id !== this.props.id ) {
if ( this.contextWatchdog ) {
this.contextWatchdog.destroy();
}

this._initializeContextWatchdog( nextProps.config );
return true;
}

// Rerender the component once again if the layout is ready.
if ( nextProps.isLayoutReady && !this.contextWatchdog ) {
this._initializeContextWatchdog( nextProps.config );

return true;
}

// Rerender the component only when children has changed.
return this.props.children !== nextProps.children;
}

/**
* Re-render the entire component once again. The old editor will be destroyed and the new one will be created.
*/
componentDidUpdate() {
// If the `isLayoutReady` property has changed from `false` to `true`, the instance of `ContextWatchdog` does not exist.
if ( this.contextWatchdog ) {
this.contextWatchdog.destroy();
}

this._initializeContextWatchdog( this.props.config );
pomek marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Destroy the context before unmounting the component.
*/
componentWillUnmount() {
this._destroyContext();
}

/**
* @return {JSX.Element}
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH it's the React Component API. We don't need to document these methods 😅

*/
render() {
return (
<ContextWatchdogContext.Provider value={ this.contextWatchdog } >
Expand All @@ -49,14 +74,13 @@ export default class CKEditorContext extends React.Component {
);
}

componentWillUnmount() {
this._destroyContext();
}

_initializeContextWatchdog( config ) {
/**
* @private
*/
_initializeContextWatchdog() {
this.contextWatchdog = new ContextWatchdog( this.props.context );

this.contextWatchdog.create( config )
this.contextWatchdog.create( this.props.config )
.catch( error => {
this.props.onError( error, {
phase: 'initialization',
Expand All @@ -78,6 +102,9 @@ export default class CKEditorContext extends React.Component {
} );
}

/**
* @private
*/
_destroyContext() {
if ( this.contextWatchdog ) {
this.contextWatchdog.destroy();
Expand All @@ -88,14 +115,14 @@ export default class CKEditorContext extends React.Component {

CKEditorContext.defaultProps = {
isLayoutReady: true,
onError: ( error, details ) => console.error( error, details )
onError: console.error
};

// Properties definition.
CKEditorContext.propTypes = {
context: PropTypes.func.isRequired,
id: PropTypes.string,
isLayoutReady: PropTypes.bool,
context: PropTypes.func,
config: PropTypes.object,
onReady: PropTypes.func,
onError: PropTypes.func
Expand Down
40 changes: 40 additions & 0 deletions tests/_utils-tests/context.js
@@ -0,0 +1,40 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

import Context from '../_utils/context';

describe( 'Context', () => {
describe( 'constructor()', () => {
it( 'saves the config', () => {
const config = { foo: 'bar' };
const context = new Context( config );

expect( context.config ).to.equal( config );
} );
} );

describe( 'destroy()', () => {
it( 'should return a promise that resolves properly', () => {
return Context.create()
.then( context => {
const promise = context.destroy();

expect( promise ).to.be.an.instanceof( Promise );

return promise;
} );
} );
} );

describe( 'create()', () => {
it( 'should return a promise that resolves properly', () => {
const promise = Context.create();

expect( promise ).to.be.an.instanceof( Promise );

return promise;
} );
} );
} );
25 changes: 14 additions & 11 deletions tests/_utils-tests/turnoffdefaulterrorcatching.js
@@ -1,19 +1,22 @@
/**
* @license Copyright (c) 2003-2021, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
* For licensing, see LICENSE.md.
*/

/* global window */

/**
* Turns off the default error catching
* so Mocha won't complain about errors caused by the called function.
*/
export default async function turnOffDefaultErrorCatching( fn ) {
const originalOnError = window.onerror;
window.onerror = null;
import turnOffDefaultErrorCatching from '../_utils/turnoffdefaulterrorcatching';

describe( 'turnOffDefaultErrorCatching()', () => {
it( 'should catch the error', () => {
const onErrorStub = sinon.stub( window, 'onerror' );

turnOffDefaultErrorCatching( () => {
window.onerror( 'Foo', null, 0 );
} );

await fn();
onErrorStub.restore();

window.onerror = originalOnError;
}
expect( onErrorStub.called ).to.equal( false );
} );
} );