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

Remove memoise withColors; Simplify API. #6782

Merged
merged 4 commits into from Jun 19, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented May 16, 2018

This PR simplify the withColors HOC so we can avoid the usage of memoize while still having a correct implementation without unnecessary rerenders.
It is based on a solution proposed by @aduth in #6686 (comment). Unfortunately passing a single parameter as suggested e.g.:withColors( 'color' ), was not possible for all cases so this API uses three parameters. I think we may add a single parameter version in the future. It may handle this case withColors( 'backgroundColor', 'customBackgroundColor', 'background-color' ), as everything can be derived from backgroundColor but not this case `withColors( 'textColor', 'customTextColor', 'color' ),. So having the three parameters version is a must.

How has this been tested?

No noticeable changes are expected. Verify setting colors (and custom colors) in paragraph and button still work as expected. Use the Highlight updates browser feature and check no unexpected rerenders are happening (when compared to master).

Revert changes in paragraph and button and test things still work as before, (we are backcompatible)

@jorgefilipecosta jorgefilipecosta self-assigned this May 16, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core May 22, 2018

@aduth

This comment has been minimized.

Member

aduth commented May 24, 2018

Unfortunately passing a single parameter as suggested e.g.:withColors( 'color' ), was not possible for all cases

Can you elaborate on what was not possible?

colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
};
} ),
( WrappedComponent ) => {

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

Style: The trailing parentheses placement on the previous line makes it difficult to discern this as the second argument of withSelect or the second entry in the array.

I'd suggest either:

withSelect( ( select ) => {
	const settings = select( 'core/editor' ).getEditorSettings();
	return {
		colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
	};
} ),

Or:

withSelect(
	( select ) => {
		const settings = select( 'core/editor' ).getEditorSettings();
		return {
			colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
		};
	}
),
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 25, 2018

Hi @aduth thank you for the review!

Unfortunately passing a single parameter as suggested, e.g., withColors( 'color' ), was not possible for all cases

Can you elaborate on what was not possible?

In this case: withColors( 'backgroundColor', 'customBackgroundColor', 'background-color' ), we can use one paramenter withColors( 'backgroundColor' ) and from the string backgroundColor we can compute the strings customBackgroundColor and background-color, they are just string appends and transformations without any map.

In this case: withColors( 'textColor', 'customTextColor', 'color' ), no direct systematic string transformation can be applied that works exactly for this case and the previous case. From 'textColor' generating 'color' seems like a hardcoded map and not a systematic string transformation.
On a second, The CSS docs say The color CSS property sets the foreground color value of an element's text content and text decorations., so mapping, textColor to color may not be something wrong, and I'm open to adding this hardcoded map. Although even if we add the one attribute approach, I would like to offer also the three attributes approach to have a flexible mechanism.

@aduth

This comment has been minimized.

Member

aduth commented May 29, 2018

Thanks for the explanation. It reveals a couple more questions:

  • Why do we call the attribute textColor if the style to which it maps is called color ?
  • Even if we support a mapping, is it enough to have the base attribute name (e.g. textColor), to which custom is prepended for the third argument?
    • One advantage here is that it enables using object syntax, especially useful for multiple color support, e.g.
withColors( {
	backgroundColor: 'backgroundColor',
	textColor: 'color',
} );

// Or: (in a somewhat `classnames` fashion)

withColors( 'backgroundColor', {
	textColor: 'color',
} );
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 30, 2018

Hi @aduth,

Why do we call the attribute textColor if the style to which it maps is called color ?

I'm not certain about this one, as far as I know in the paragraph we always used textColor + backgroundColor. In button, we used color at the start because we had no textColor. Color, was the background color of the button. When textColor was added I think we started using backgroundColor + textColor.
I like the names as they are now because textColor and backgroundColor clearly specify where colors are applied in the context of that block. For example, the color attribute in button may not be easily associated with textColor (and in fact, it was already used to reference background color).

Even if we support a mapping, is it enough to have the base attribute name (e.g. textColor), to which custom is prepended for the third argument?
One advantage here is that it enables using object syntax, especially useful for multiple color support, e.g.

I like this suggestion, I added a new commit that allows us to use a "classnames" like syntax, and allows us to use the simple syntax e.g: withColors( 'backgroundColor' );

@aduth

I'm liking the new API 👍

};
} ),
edit: compose( [
withColors( 'backgroundColor', { textColor: 'color' } ),
FallbackStyles,

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

Unrelated: I don't think this should be capitalized as though it were a component, since it's not. Confused me thinking we had incorrect usage. Something with a with prefix would be good to reflect it being a higher-order component (admitting there is a bit of confusion on higher-order-components vs. higher-order-component-creators like withFallbackStyles).

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

It seems withFallbackStyles is a better name I will rename it in a separate PR.

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Filed #7355 so we don't forget (plus it's a good first bug).

setTextColor: setColor( 'textColor', 'customTextColor' ),
};
} )( ButtonEdit );
export default compose( [

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

We don't need compose?

export default withColors( 'backgroundColor', { textColor: 'color' } )( ButtonEdit );
* @param {Function} mapGetSetColorToProps Function that receives getColor, setColor, and props,
* and returns additional props to pass to the component.
* @param {...(object|string)} args The arguments can be strings or objects. If the argument is an object,
* it should contain the color attribute name as key and the color context as value.

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

Can we define somewhere what we mean by "color context" in practice? Each time I look at this, I forget what it's for. I see it's primarily used as part of the generated class name modifier e.g. has-green-color ?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

I extended the comment to describe the color context.

);
}
mergePropsAcc[ `set${ ufColorAttrName }` ] =
setColorValue( colors, colorAttributeName, customColorAttributeName, setAttributes );

This comment has been minimized.

@aduth

aduth May 30, 2018

Member

Rather than copying setAttributes from props into state, could instead we assign the prop callback as an instance-bound function?

Similar to what we do in withDispatch:

this.proxyProps = mapValues( propsToDispatchers, ( dispatcher, propName ) => {
// Prebind with prop name so we have reference to the original
// dispatcher to invoke. Track between re-renders to avoid
// creating new function references every render.
if ( this.proxyProps.hasOwnProperty( propName ) ) {
return this.proxyProps[ propName ];
}
return this.proxyDispatch.bind( this, propName );
} );

Getting a general sense we might be trying to hard to optimize this with caching copies of relevantAttributes, colors, setAttributes. Maybe we could compare against prevState.mergeProps to see if the attribute value has changed, rather than doing the shallow equality before.

Something like:

return class extends Component {
	constructor( props ) {
		super( props );
		this.state = {};
	}

	static getDerivedStateFromProps( { attributes, colors, setAttributes }, prevState ) {
		return reduce( colorMap, ( mergePropsAcc, colorContext, colorAttributeName ) => {
			const ufColorAttrName = upperFirst( colorAttributeName );
			const customColorAttributeName = `custom${ ufColorAttrName }`;
			const colorName = attributes[ colorAttributeName ];
			const customColor = attributes[ customColorAttributeName ];
			const colorValue = getColorValue( colors, colorName, customColor );
			if ( get( mergePropsAcc, [ colorAttributeName, 'value' ] ) === colorValue ) {
				return mergePropsAcc;
			}

			mergePropsAcc[ colorAttributeName ] = {
				name: colorName,
				class: getColorClass( colorContext, colorName ),
				value: colorValue,
			};
			mergePropsAcc[ `set${ ufColorAttrName }` ] =
				setColorValue( colors, colorAttributeName, customColorAttributeName, setAttributes );
			return mergePropsAcc;
		}, prevState );
	}

	render() {
		return (
			<WrappedComponent
				{ ...{
					...this.props,
					colors: undefined,
					...this.state,
				} }
			/>
		);
	}
};

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Hi @aduth, I changed the code in the direction of your feedback.

'withColors'
);
createSetColor( colorAttributeName, customColorAttributeName ) {
return ( ( colorValue ) => {

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

There's a tab here after the space following return. Not sure why ESLint didn't pick it up.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 6, 2018

Member

Nice catch I made my editor show tabs and spaces all the time, to avoid this problem in the future :)

[ colorAttributeName ]: colorObj && colorObj.name ? colorObj.name : undefined,
[ customColorAttributeName ]: colorObj && colorObj.name ? undefined : colorValue,
} );
} ).bind( this );

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

Is bind valid for an arrow function? Is it needed?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 6, 2018

Member

No needed, arrow functions cannot be rebinded and the bind for this is automatic. It was corrected 👍

);
const prevColorObject = prevState[ colorAttributeName ];
const prevColorValue = get( prevColorObject, [ 'value' ] );
if ( prevColorValue === colorValue && prevColorValue ) {

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

For what case do we need && prevColorValue ? Can we have a unit test?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 6, 2018

Member

It is a trivial case and the condition is executed all the time, without && prevColorValue during the first execution, both prevColorValue and colorValue are undefined. So we would always execute newState[ colorAttributeName ] = prevColorObject; and never compute the object. I will add this as a comment to make clear why && prevColorValue is needed.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 6, 2018

Member

I also updated the code to use && prevColorObject, this makes the condition more clear.

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Jun 18, 2018

@tofumatt

I have my usual nitpicky feelings about abbreviated variables names and comments, but this looks good to me assuming little nitpicks are cleaned up.

Maybe @aduth wants to have another look too though, as he did the original reviews. 🤷‍♂️

constructor() {
super( ...arguments );
/**
* Even though, we don't expect setAttributes or colors to change memoizing it is essential.

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

There shouldn't be a comma after "though".

* @param {...(object|string)} args The arguments can be strings or objects. If the argument is an object,
* it should contain the color attribute name as key and the color context as value.
* If the argument is a string the value should be the color attribute name,
* the color context is computed by applying a kebab case transform to the value.

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Haha, I've never heard of kebab case 😄 🍖

/**
* Internal dependencies
*/
import { getColorValue, getColorClass, setColorValue } from './utils';
import { getColorValue, getColorClass } from './utils';
import { default as withColorsDeprecated } from './with-colors-deprecated';

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Why is this import { default as withColorsDeprecated } from './with-colors-deprecated'; instead of just import withColorsDeprecated from './with-colors-deprecated';?

}
createSetters() {
return reduce( colorMap, ( settersAcc, colorContext, colorAttributeName ) => {
const ufColorAttrName = upperFirst( colorAttributeName );

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

ufColorAttrName is a lot of abbreviation–I'm not sure what it means and I tried to read above but it was hard to tell from context.

);
}
createSetters() {
return reduce( colorMap, ( settersAcc, colorContext, colorAttributeName ) => {

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

What does Acc stand for? Sorry, a lot of these abbreviations are totally new to me and might throw other new contributors off.

This comment has been minimized.

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Ah, it would be nice to expand the variable name then, that is a lot more instructive 🤷‍♂️

This comment has been minimized.

@aduth

aduth Jun 19, 2018

Member

Also noting that we have a horrible inconsistency with our naming of the Array#reduce accumulator variable throughout the codebase, often also referred to as memo or result.

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

Should we follow Mozilla docs?

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

I filed #7378 for it; setting it to accumulator everywhere works for me; we could do something like settersAccumulator here and elsewhere if we want more clarity and I think that'd be fine.

};
}
static getDerivedStateFromProps( { attributes, colors }, prevState ) {

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

I also think we have space for previous instead of prev 😉

const prevColorObject = prevState[ colorAttributeName ];
const prevColorValue = get( prevColorObject, [ 'value' ] );
/**
* && prevColorObject checks that a previous color object was already computed.

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

What is the && here for?

if ( isFunction( args[ 0 ] ) ) {
deprecated( 'Using withColors( mapGetSetColorToProps ) ', {
version: '3.2',
alternative: 'withColors( colorAttributeName, { secondColorAttributeName: \'color-context\' }, ... )',

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

It would be nicer to use double-quotes here rather than escaping the single-quotes inside. Is that against the style rules? (If so: ouch. 😅)

const colorMap = reduce( args, ( colorObj, arg ) => {
return {
...colorObj,

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Obj strikes again 😉

};
} ),
edit: compose( [
withColors( 'backgroundColor', { textColor: 'color' } ),
FallbackStyles,

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Filed #7355 so we don't forget (plus it's a good first bug).

@jorgefilipecosta jorgefilipecosta added this to the 3.1 milestone Jun 19, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 19, 2018

Merging this so we have more time and testing before the next release. If there is any other improvement that we find would improve the code/API I will gladly open a follow-up PR.

@jorgefilipecosta jorgefilipecosta merged commit 7f1c8d3 into master Jun 19, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +11.85% compared to 14531c4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/remove-memoise-withColors branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment