Merge pull request #81 from ckeditor/i/79

Fix: Improved performance of the inspector by avoiding unnecessary React rendering. Closes #79.
ckeditor · May 27, 2020 · 44e7850 · 44e7850
2 parents 65b8aca + 9f0b53d
commit 44e7850
Show file tree

Hide file tree

Showing 20 changed files with 256 additions and 150 deletions.
diff --git a/package.json b/package.json
@@ -1,6 +1,5 @@
 {
   "name": "@ckeditor/ckeditor5-inspector",
-  "dependencies": {},
   "main": "build/inspector.js",
   "files": [
     "build"
@@ -81,6 +80,7 @@
     "prop-types": "^15.7.2",
     "react": "^16.8.1",
     "react-dom": "^16.8.1",
+    "react-fast-compare": "^3.1.1",
     "react-redux": "^7.2.0",
     "react-rnd": "^10.0.0",
     "redux": "^4.0.5",

diff --git a/src/commands/commandinspector.js b/src/commands/commandinspector.js
@@ -13,6 +13,21 @@ import ObjectInspector from '../components/objectinspector';
 import Logger from '../logger';
 
 class CommandInspector extends Component {
+	constructor( props ) {
+		super( props );
+
+		this.handleCommandLogButtonClick = this.handleCommandLogButtonClick.bind( this );
+		this.handleCommandExecuteButtonClick = this.handleCommandExecuteButtonClick.bind( this );
+	}
+
+	handleCommandLogButtonClick() {
+		Logger.log( this.props.currentCommandDefinition.command );
+	}
+
+	handleCommandExecuteButtonClick() {
+		this.props.editors.get( this.props.currentEditorName ).execute( this.props.currentCommandName );
+	}
+
 	render() {
 		const definition = this.props.currentCommandDefinition;
 
@@ -22,8 +37,6 @@ class CommandInspector extends Component {
 			</Pane>;
 		}
 
-		const currentEditor = this.props.editors.get( this.props.currentEditorName );
-
 		return <ObjectInspector
 			header={[
 				<span key="link">
@@ -36,13 +49,13 @@ class CommandInspector extends Component {
 					key="exec"
 					type="exec"
 					text="Execute command"
-					onClick={() => currentEditor.execute( this.props.currentCommandName )}
+					onClick={this.handleCommandExecuteButtonClick}
 				/>,
 				<Button
 					key="log"
 					type="log"
 					text="Log in console"
-					onClick={() => Logger.log( definition.command )}
+					onClick={this.handleCommandLogButtonClick}
 				/>
 			]}
 			lists={[

diff --git a/src/components/button.js b/src/components/button.js
@@ -3,10 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
-import React, { Component } from 'react';
+import React, { PureComponent } from 'react';
 import './button.css';
 
-export default class Button extends Component {
+export default class Button extends PureComponent {
 	render() {
 		return <button
 			className={`ck-inspector-button ck-inspector-button_${ this.props.type }`}

diff --git a/src/components/checkbox.js b/src/components/checkbox.js
@@ -3,10 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
-import React, { Component } from 'react';
+import React, { PureComponent } from 'react';
 import './checkbox.css';
 
-export default class Checkbox extends Component {
+export default class Checkbox extends PureComponent {
 	render() {
 		return [
 			<input type="checkbox"

diff --git a/src/components/objectinspector.js b/src/components/objectinspector.js
@@ -3,12 +3,12 @@
  * For licensing, see LICENSE.md.
  */
 
-import React, { Component } from 'react';
+import React, { PureComponent } from 'react';
 import PropertyList from './propertylist';
 import Button from './button';
 import './objectinspector.css';
 
-export default class ObjectInspector extends Component {
+export default class ObjectInspector extends PureComponent {
 	render() {
 		const content = [];
 
@@ -27,6 +27,7 @@ export default class ObjectInspector extends Component {
 				</h3>,
 				<PropertyList
 					key={`${ list.name }-list`}
+					name={list.name}
 					itemDefinitions={list.itemDefinitions}
 					presentation={list.presentation}
 				/>

diff --git a/src/components/propertylist.js b/src/components/propertylist.js
@@ -3,15 +3,15 @@
  * For licensing, see LICENSE.md.
  */
 
-import React, { Component } from 'react';
-import { uid, truncateString } from './utils';
+import React, { PureComponent, Component } from 'react';
+import isEqual from 'react-fast-compare';
+import { truncateString } from './utils';
 import './propertylist.css';
 
 const MAX_PROPERTY_VALUE_LENGTH = 2000;
 
 export default class PropertyList extends Component {
 	render() {
-		const listUid = uid();
 		const expandCollapsibles = this.props.presentation && this.props.presentation.expandCollapsibles;
 		const children = [];
 
@@ -23,16 +23,16 @@ export default class PropertyList extends Component {
 
 			const itemChildren = [
 				<PropertyTitle
-					key={`${ this.props.name }-name`}
+					key={`${ this.props.name }-${ name }-name`}
 					name={name}
-					listUid={listUid}
+					listUid={this.props.name}
 					canCollapse={hasSubProperties}
 					colorBox={presentation.colorBox}
 					expandCollapsibles={expandCollapsibles}
 				/>,
-				<dd key={`${ name }-value`}>
+				<dd key={`${ this.props.name }-${ name }-value`}>
 					<input
-						id={`${ listUid }-${ name }-input`}
+						id={`${ this.props.name }-${ name }-value-input`}
 						type="text"
 						value={value}
 						readOnly={true}
@@ -43,7 +43,8 @@ export default class PropertyList extends Component {
 			if ( hasSubProperties ) {
 				itemChildren.push(
 					<PropertyList
-						key={`${ name }-subProperties`}
+						name={`${ this.props.name }-${ name }`}
+						key={`${ this.props.name }-${ name }`}
 						itemDefinitions={subProperties}
 						presentation={this.props.presentation}
 					/>
@@ -57,9 +58,13 @@ export default class PropertyList extends Component {
 			{children}
 		</dl>;
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
 
-class PropertyTitle extends Component {
+class PropertyTitle extends PureComponent {
 	constructor( props ) {
 		super( props );
 
@@ -93,7 +98,7 @@ class PropertyTitle extends Component {
 		return <dt className={classNames.join( ' ' ).trim()}>
 			{collapseButton}
 			{colorBox}
-			<label htmlFor={`${ this.props.listUid }-${ this.props.name }-input`}>
+			<label htmlFor={`${ this.props.listUid }-${ this.props.name }-value-input`}>
 				{this.props.name}
 			</label>:
 		</dt>;

diff --git a/src/components/select.js b/src/components/select.js
@@ -4,6 +4,7 @@
  */
 
 import React, { Component } from 'react';
+import isEqual from 'react-fast-compare';
 
 export default class Select extends Component {
 	render() {
@@ -21,4 +22,8 @@ export default class Select extends Component {
 			</select>
 		];
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
diff --git a/src/components/tree/treeelement.js b/src/components/tree/treeelement.js
@@ -4,7 +4,7 @@
  */
 
 import React from 'react';
-
+import isEqual from 'react-fast-compare';
 import TreeNode from './treenode';
 import TreeNodeAttribute from './treenodeattribute';
 import TreePosition from './treeposition';
@@ -86,4 +86,8 @@ export default class TreeElement extends TreeNode {
 
 		return attributes;
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
diff --git a/src/components/tree/treenode.js b/src/components/tree/treenode.js
@@ -5,6 +5,7 @@
 
 import { Component } from 'react';
 import { renderTreeNodeFromDefinition } from './utils';
+import isEqual from 'react-fast-compare';
 
 /**
  * A base class for nodes in the tree.
@@ -37,4 +38,8 @@ export default class TreeNode extends Component {
 	get isActive() {
 		return this.definition.node === this.globalTreeProps.activeNode;
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
diff --git a/src/components/tree/treenodeattribute.js b/src/components/tree/treenodeattribute.js
@@ -3,15 +3,15 @@
  * For licensing, see LICENSE.md.
  */
 
-import React, { Component } from 'react';
+import React, { PureComponent } from 'react';
 import { truncateString } from '../utils';
 
 const MAX_ATTRIBUTE_VALUE_LENGTH = 500;
 
 /**
  * A class which instances represent attributes in the tree.
  */
-export default class TreeNodeAttribute extends Component {
+export default class TreeNodeAttribute extends PureComponent {
 	render() {
 		let valueElement;
 		const value = truncateString( this.props.value, MAX_ATTRIBUTE_VALUE_LENGTH );

diff --git a/src/components/tree/treeposition.js b/src/components/tree/treeposition.js
@@ -4,6 +4,7 @@
  */
 
 import React, { Component } from 'react';
+import isEqual from 'react-fast-compare';
 
 /**
  * A class which instances represent positions (selection, markers) in the tree.
@@ -31,4 +32,8 @@ export default class TreePosition extends Component {
 
 		return <span {...attrs}>&#8203;</span>;
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
diff --git a/src/components/tree/treetextnode.js b/src/components/tree/treetextnode.js
@@ -4,6 +4,7 @@
  */
 
 import React from 'react';
+import isEqual from 'react-fast-compare';
 
 import TreeNode from './treenode';
 import TreeNodeAttribute from './treenodeattribute';
@@ -25,7 +26,9 @@ export default class TreeTextNode extends TreeNode {
 		if ( definition.positions && definition.positions.length ) {
 			nodeText = nodeText.split( '' );
 
-			definition.positions
+			// Don't alter the props (sort&reverse would do that in place) because next time it will cause unnecessary
+			// rendering due to equality check indicating old and new props are different arrays.
+			Array.from( definition.positions )
 				.sort( ( posA, posB ) => {
 					if ( posA.offset < posB.offset ) {
 						return -1;
@@ -83,4 +86,8 @@ export default class TreeTextNode extends TreeNode {
 			{attributes}
 		</span>;
 	}
+
+	shouldComponentUpdate( nextProps ) {
+		return !isEqual( this.props, nextProps );
+	}
 }
diff --git a/src/model/nodeinspector.js b/src/model/nodeinspector.js
@@ -13,6 +13,16 @@ import ObjectInspector from '../components/objectinspector';
 import Logger from '../logger';
 
 class ModelNodeInspector extends Component {
+	constructor( props ) {
+		super( props );
+
+		this.handleNodeLogButtonClick = this.handleNodeLogButtonClick.bind( this );
+	}
+
+	handleNodeLogButtonClick() {
+		Logger.log( this.props.currentNodeDefinition.editorNode );
+	}
+
 	render() {
 		const definition = this.props.currentNodeDefinition;
 
@@ -34,7 +44,7 @@ class ModelNodeInspector extends Component {
 					key="log"
 					type="log"
 					text="Log in console"
-					onClick={() => Logger.log( definition.editorNode )}
+					onClick={this.handleNodeLogButtonClick}
 				/>
 			]}
 			lists={[