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

Columns: Avoid re-render and subsequent action dispatch by adopting module constant #7720

Merged
merged 1 commit into from Jul 5, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jul 5, 2018

We were passing a new allowedBlocks reference on each column render. This dispatch additional unnecessary further actions to update inner block settings.
allowedBlocks reference should only be changed if, in fact, we are changing the allowedBlocks.
In the columns block, we can make allowed block a constant given that it never changes.

This problem may cause noticeable lag when writing/using columns because updating the InnerBlock setting may cause another rerender and another rerender updates the settings again. So this change prevents cascade re-renders.

The alternative is comparing each element of the array to verify if it changed or not. But this has a performance impact and we should not pass new props during rerenders if the props did not change so I feel this is the correct approach.

How has this been tested?

I verified the columns block works as before and no unnecessary UPDATE_BLOCK_LIST_SETTINGS are being dispatched.

Improve the performance of the columns block
We were passing a new allowedBlocks reference on each column render. This dispatch additional unnecessary further actions to update inner block settings.
allowedBlocks reference should only be changed if, in fact, we are changing the allowedBlocks.
In the columns block, we can make allowed block a  constant given that it never changes.

@jorgefilipecosta jorgefilipecosta requested a review from aduth Jul 5, 2018

@mcsf

mcsf approved these changes Jul 5, 2018

@mcsf mcsf changed the title from Improve the performance of the columns block to Columns: Avoid re-render and subsequent action dispatch by adopting module constant Jul 5, 2018

@jorgefilipecosta jorgefilipecosta merged commit 622bbfb into master Jul 5, 2018

2 checks passed

codecov/project 46.43% (+<.01%) compared to 9a23989
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/improve-performace-of-columns-block branch Jul 5, 2018

@mtias mtias added this to the 3.2 milestone Jul 5, 2018

@mtias mtias added the Performance label Jul 5, 2018

@jorgefilipecosta jorgefilipecosta self-assigned this Jul 5, 2018

@@ -23,6 +23,8 @@ import {
import './style.scss';
import './editor.scss';
const ALLOWED_BLOCKS = [ 'core/column' ];

This comment has been minimized.

@aduth

aduth Jul 10, 2018

Member

Should document constants with JSDoc.

Edit: I could see the argument being made that this is pretty self-explanatory, which I'd likely agree with for the most part, but it's also about setting a convention for other developers to adopt: Always document all the things, because it won't always be so obvious. Also with JSDoc typings, we can be proactive about encouraging the next maintainer to know the shape of value the assignment is meant to be supporting.

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