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.

Sign up for GitHub

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

Jump to bottom

Fix RichText rerendering when it shouldn't #12161

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Fix RichText rerendering when it shouldn't #12161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
254cf33
Fix RichText rerendering when it shouldn't
atimmer Nov 21, 2018
2fc48e9
Rename emptyArray to EMPTY_ARRAY
atimmer Nov 21, 2018
cb37598
Add memize as a dependency to annotations package
atimmer Nov 21, 2018
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
55 changes: 41 additions & 14 deletions packages/annotations/src/format/annotation.js
View file
Open in desktop
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
6 changes: 4 additions & 2 deletions packages/annotations/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import createSelector from 'rememo';
import { get, flatMap } from 'lodash';

const emptyArray = [];
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I think in other places we called this EMPTY_ARRAY.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be bad to have the same comment:

gutenberg/packages/editor/src/store/selectors.js

Lines 57 to 66 in 563ef41

/**
* 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 = [];

Copy link
Member

Choose a reason for hiding this comment

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

Minor: I think in other places we called this EMPTY_ARRAY.

Related guideline: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#constants


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

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

Expand Down
25 changes: 17 additions & 8 deletions packages/rich-text/src/register-format-type.js
View file
Open in desktop
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 Down Expand Up @@ -119,6 +120,14 @@ export function registerFormatType( name, settings ) {

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

const emptyArray = [];
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

const getFunctionStackMemoized = memize( ( previousStack = emptyArray, newFunction ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Would it work to keep the cache size to one?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why do both this and prepareEditableTree need to be memoized? Could use some more clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not as we need one per RichText I think.

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 don't think so, because a different function could be returned based on richTextIdentifier/blockClientId.

Copy link
Member

Choose a reason for hiding this comment

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

Just wondering because it's probably best to keep cache size as small as possible. I'm guessing the issue is just consecutive calls and nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

So how big is this cache going to grow?

Copy link
Member

Choose a reason for hiding this comment

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

No way to cache per instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question though

return [
...previousStack,
newFunction,
];
} );

if (
settings.__experimentalGetPropsForEditableTreePreparation
) {
Expand All @@ -133,13 +142,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 +164,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