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

Annotations & Performance: remove use of Memize #14664

Merged
merged 1 commit into from
Apr 3, 2019
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/annotations/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
"@wordpress/i18n": "file:../i18n",
"@wordpress/rich-text": "file:../rich-text",
"lodash": "^4.17.11",
"memize": "^1.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

package-lock.json should have been updated as a result of this change. There's local revisions after running npm install in master.

Aside: Travis should be catching this, and it's unclear why it's not.

Copy link
Member

Choose a reason for hiding this comment

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

See #14808

"rememo": "^3.0.0",
"uuid": "^3.3.2"
},
Expand Down
55 changes: 14 additions & 41 deletions packages/annotations/src/format/annotation.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import memize from 'memize';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -120,40 +115,6 @@ function updateAnnotationsWithPositions( annotations, positions, { removeAnnotat
} );
}

/**
* Create prepareEditableTree memoized based on the annotation props.
*
* @param {Object} The props with annotations in them.
*
* @return {Function} The prepareEditableTree.
*/
const createPrepareEditableTree = memize( ( props ) => {
const { annotations } = props;

return ( formats, text ) => {
if ( annotations.length === 0 ) {
return formats;
}

let record = { formats, text };
record = applyAnnotations( record, annotations );
return record.formats;
};
} );

/**
* Returns the annotations as a props object. Memoized to prevent re-renders.
*
* @param {Array} The annotations to put in the object.
*
* @return {Object} The annotations props object.
*/
const getAnnotationObject = memize( ( annotations ) => {
return {
annotations,
};
} );

export const annotation = {
name: FORMAT_NAME,
title: __( 'Annotation' ),
Expand All @@ -167,9 +128,21 @@ export const annotation = {
return null;
},
__experimentalGetPropsForEditableTreePreparation( select, { richTextIdentifier, blockClientId } ) {
return getAnnotationObject( select( STORE_KEY ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ) );
return {
annotations: select( STORE_KEY ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ),
};
},
__experimentalCreatePrepareEditableTree( { annotations } ) {
return ( formats, text ) => {
if ( annotations.length === 0 ) {
return formats;
}

let record = { formats, text };
record = applyAnnotations( record, annotations );
return record.formats;
};
},
__experimentalCreatePrepareEditableTree: createPrepareEditableTree,
__experimentalGetPropsForEditableTreeChangeHandler( dispatch ) {
return {
removeAnnotation: dispatch( STORE_KEY ).__experimentalRemoveAnnotation,
Expand Down
78 changes: 32 additions & 46 deletions packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import {
isEqual,
omit,
pickBy,
get,
isPlainObject,
} from 'lodash';
import memize from 'memize';

Expand Down Expand Up @@ -48,6 +46,7 @@ import {
import { decodeEntities } from '@wordpress/html-entities';
import { withFilters, IsolatedEventContainer } from '@wordpress/components';
import deprecated from '@wordpress/deprecated';
import isShallowEqual from '@wordpress/is-shallow-equal';

/**
* Internal dependencies
Expand Down Expand Up @@ -91,6 +90,20 @@ const globalStyle = document.createElement( 'style' );

document.head.appendChild( globalStyle );

function createPrepareEditableTree( props ) {
const fns = Object.keys( props ).reduce( ( accumulator, key ) => {
if ( key.startsWith( 'format_prepare_functions' ) ) {
accumulator.push( props[ key ] );
}

return accumulator;
}, [] );

return ( value ) => fns.reduce( ( accumulator, fn ) => {
return fn( accumulator, value.text );
}, value.formats );
}

export class RichText extends Component {
constructor( { value, onReplace, multiline } ) {
super( ...arguments );
Expand Down Expand Up @@ -202,7 +215,7 @@ export class RichText extends Component {
range,
multilineTag: this.multilineTag,
multilineWrapperTags: this.multilineWrapperTags,
prepareEditableTree: this.props.prepareEditableTree,
prepareEditableTree: createPrepareEditableTree( this.props ),
__unstableIsEditableTree: true,
} );
}
Expand All @@ -213,7 +226,7 @@ export class RichText extends Component {
current: this.editableRef,
multilineTag: this.multilineTag,
multilineWrapperTags: this.multilineWrapperTags,
prepareEditableTree: this.props.prepareEditableTree,
prepareEditableTree: createPrepareEditableTree( this.props ),
__unstableDomOnly: domOnly,
} );
}
Expand Down Expand Up @@ -484,18 +497,6 @@ export class RichText extends Component {
}
}

/**
* Calls all registered onChangeEditableValue handlers.
*
* @param {Array} formats The formats of the latest rich-text value.
* @param {string} text The text of the latest rich-text value.
*/
onChangeEditableValue( { formats, text } ) {
get( this.props, [ 'onChangeEditableValue' ], [] ).forEach( ( eventHandler ) => {
eventHandler( formats, text );
} );
}

/**
* Sync the value to global state. The node tree and selection will also be
* updated if differences are found.
Expand All @@ -509,8 +510,14 @@ export class RichText extends Component {
this.applyRecord( record );

const { start, end, activeFormats = [] } = record;
const changeHandlers = pickBy( this.props, ( v, key ) =>
key.startsWith( 'format_on_change_functions_' )
);

Object.values( changeHandlers ).forEach( ( changeHandler ) => {
changeHandler( record.formats, record.text );
} );

this.onChangeEditableValue( record );
this.savedContent = this.valueToFormat( record );
this.props.onChange( this.savedContent );
this.setState( { start, end, activeFormats } );
Expand Down Expand Up @@ -912,23 +919,13 @@ export class RichText extends Component {
this.savedContent = value;
}

// If any format props update, reapply value.
const shouldReapply = Object.keys( this.props ).some( ( name ) => {
if ( name.indexOf( 'format_' ) !== 0 ) {
return false;
}

// Allow primitives and arrays:
if ( ! isPlainObject( this.props[ name ] ) ) {
return this.props[ name ] !== prevProps[ name ];
}

return Object.keys( this.props[ name ] ).some( ( subName ) => {
return this.props[ name ][ subName ] !== prevProps[ name ][ subName ];
} );
} );
const prefix = 'format_prepare_props_';
const predicate = ( v, key ) => key.startsWith( prefix );
const prepareProps = pickBy( this.props, predicate );
const prevPrepareProps = pickBy( prevProps, predicate );

if ( shouldReapply ) {
// If any format prepare props update, reapply value.
if ( ! isShallowEqual( prepareProps, prevPrepareProps ) ) {
const record = this.formatToValue( value );

// Maintain the previous selection if the instance is currently
Expand All @@ -942,15 +939,6 @@ export class RichText extends Component {
}
}

/**
* Get props that are provided by formats to modify RichText.
*
* @return {Object} Props that start with 'format_'.
*/
getFormatProps() {
return pickBy( this.props, ( propValue, name ) => name.startsWith( 'format_' ) );
}

/**
* Converts the outside data structure to our internal representation.
*
Expand Down Expand Up @@ -988,7 +976,7 @@ export class RichText extends Component {
return unstableToDom( {
value,
multilineTag: this.multilineTag,
prepareEditableTree: this.props.prepareEditableTree,
prepareEditableTree: createPrepareEditableTree( this.props ),
} ).body.innerHTML;
}

Expand Down Expand Up @@ -1066,9 +1054,7 @@ export class RichText extends Component {
const record = this.getRecord();

return (
<div className={ classes }
onFocus={ this.setFocusedElement }
>
<div className={ classes } onFocus={ this.setFocusedElement }>
{ isSelected && this.multilineTag === 'li' && (
<ListEdit
onTagNameChange={ onTagNameChange }
Expand Down
1 change: 1 addition & 0 deletions packages/rich-text/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@wordpress/compose": "file:../compose",
"@wordpress/data": "file:../data",
"@wordpress/escape-html": "file:../escape-html",
"@wordpress/hooks": "file:../hooks",
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this dependency was missing.

"lodash": "^4.17.11",
"rememo": "^3.0.0"
},
Expand Down
Loading