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

Fix RichText rerendering when it shouldn't #12161

Merged
merged 3 commits into from
Nov 21, 2018
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: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/annotations/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@wordpress/i18n": "file:../i18n",
"@wordpress/rich-text": "file:../rich-text",
"lodash": "^4.17.10",
"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.

I think we also need it on the server side.

Copy link
Contributor

Choose a reason for hiding this comment

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

No we don't, there's no script for memize

Copy link
Member

Choose a reason for hiding this comment

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

Oh, cool then :)

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

Choose a reason for hiding this comment

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

I don't see this added as a dependency.


/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -115,6 +120,40 @@ 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 ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of memoizing function, I think it would be better for the registerFormatType function to do the memoization itself using a shallow comparison. This way it would work with any implementation of __experimentalGetPropsForEditableTreePreparation

That said, we're kind of running out of time, so I'm not considering this as blocking at the moment as it's a forward-compatible change.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this sounds better.

return {
annotations,
};
} );

export const annotation = {
name: FORMAT_NAME,
title: __( 'Annotation' ),
Expand All @@ -128,21 +167,9 @@ export const annotation = {
return null;
},
__experimentalGetPropsForEditableTreePreparation( select, { richTextIdentifier, blockClientId } ) {
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;
};
return getAnnotationObject( select( STORE_KEY ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ) );
},
__experimentalCreatePrepareEditableTree: createPrepareEditableTree,
__experimentalGetPropsForEditableTreeChangeHandler( dispatch ) {
return {
removeAnnotation: dispatch( STORE_KEY ).__experimentalRemoveAnnotation,
Expand Down
17 changes: 14 additions & 3 deletions packages/annotations/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@
import createSelector from 'rememo';
import { get, flatMap } from 'lodash';

/**
* Shared reference to an empty array for cases where it is important to avoid
* returning a new array reference on every invocation, as in a connected or
* other pure component which performs `shouldComponentUpdate` check on props.
* This should be used as a last resort, since the normalized data should be
* maintained by the reducer result in state.
*
* @type {Array}
*/
const EMPTY_ARRAY = [];

/**
* Returns the annotations for a specific client ID.
*
Expand All @@ -19,12 +30,12 @@ export const __experimentalGetAnnotationsForBlock = createSelector(
} );
},
( state, blockClientId ) => [
get( state, blockClientId, [] ),
get( state, blockClientId, EMPTY_ARRAY ),
]
);

export const __experimentalGetAllAnnotationsForBlock = function( state, blockClientId ) {
return get( state, blockClientId, [] );
return get( state, blockClientId, EMPTY_ARRAY );
};

/**
Expand Down Expand Up @@ -54,7 +65,7 @@ export const __experimentalGetAnnotationsForRichText = createSelector(
} );
},
( state, blockClientId ) => [
get( state, blockClientId, [] ),
get( state, blockClientId, EMPTY_ARRAY ),
]
);

Expand Down
35 changes: 27 additions & 8 deletions packages/rich-text/src/register-format-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { mapKeys } from 'lodash';
import memize from 'memize';

/**
* WordPress dependencies
Expand All @@ -10,6 +11,17 @@ import { select, dispatch, withSelect, withDispatch } from '@wordpress/data';
import { addFilter } from '@wordpress/hooks';
import { compose } from '@wordpress/compose';

/**
* Shared reference to an empty array for cases where it is important to avoid
* returning a new array reference on every invocation, as in a connected or
* other pure component which performs `shouldComponentUpdate` check on props.
* This should be used as a last resort, since the normalized data should be
* maintained by the reducer result in state.
*
* @type {Array}
*/
const EMPTY_ARRAY = [];

/**
* Registers a new format provided a unique name and an object defining its
* behavior.
Expand Down Expand Up @@ -119,6 +131,13 @@ export function registerFormatType( name, settings ) {

dispatch( 'core/rich-text' ).addFormatTypes( settings );

const getFunctionStackMemoized = memize( ( previousStack = EMPTY_ARRAY, newFunction ) => {
return [
...previousStack,
newFunction,
];
} );

if (
settings.__experimentalGetPropsForEditableTreePreparation
) {
Expand All @@ -133,13 +152,13 @@ export function registerFormatType( name, settings ) {
const additionalProps = {};

if ( settings.__experimentalCreatePrepareEditableTree ) {
additionalProps.prepareEditableTree = [
...( props.prepareEditableTree || [] ),
additionalProps.prepareEditableTree = getFunctionStackMemoized(
props.prepareEditableTree,
settings.__experimentalCreatePrepareEditableTree( props[ `format_${ name }` ], {
richTextIdentifier: props.identifier,
blockClientId: props.clientId,
} ),
];
} )
);
}

if ( settings.__experimentalCreateOnChangeEditableValue ) {
Expand All @@ -155,16 +174,16 @@ export function registerFormatType( name, settings ) {
return accumulator;
}, {} );

additionalProps.onChangeEditableValue = [
...( props.onChangeEditableValue || [] ),
additionalProps.onChangeEditableValue = getFunctionStackMemoized(
props.onChangeEditableValue,
settings.__experimentalCreateOnChangeEditableValue( {
...props[ `format_${ name }` ],
...dispatchProps,
}, {
richTextIdentifier: props.identifier,
blockClientId: props.clientId,
} ),
];
} )
);
}

return <OriginalComponent
Expand Down