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

Editor: Flatten Inserter mapSelectToProps to optimize rendering #11028

Merged
merged 2 commits into from
Oct 26, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 24, 2018

This pull request seeks to optimize the rendering of the Inserter component by avoiding the return of a new reference object which had been returned previously in each call to its withSelect mapSelectToProps. It does so by merely flattening the insertionPoint object it had been creating.

As a general rule of thumb, mapSelectToProps should never create new objects used in values of its return object.

This has the impact of reducing the number of render calls on Inserter for a new post from ~50 to 4.

Testing instructions:

This is a refactoring change. There is expected to be no effective changes.

Ensure unit tests pass.

npm run test-unit packages/editor/src/components/inserter/test/index.js

Verify basic usage of the inserter.

@aduth aduth added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Oct 24, 2018
index: rootClientId ? getBlockOrder( rootClientId ).length : insertionPoint.index,
},
layout: rootClientId ? layout : insertionPoint.layout,
index: rootClientId ? getBlockOrder( rootClientId ).length : insertionPoint.index,
Copy link
Contributor

Choose a reason for hiding this comment

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

The two lines above are different from what we had previously. As we were not assigning the insertionPoint.rootClientId before testing. Not sure if it's impactful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, you're totally right. I expect this is wrong as implemented. I'll plan to fix.

const insertionPoint = getBlockInsertionPoint();
const parentId = rootClientId || insertionPoint.rootClientId;

let index;
Copy link
Contributor

@youknowriad youknowriad Oct 26, 2018

Choose a reason for hiding this comment

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

I think this should be initiailized something like getBlockOrder( rootClientId ).length?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, it's not needed, for reasons described in the comment a few lines below (maybe more valuable to pull up?)

// Otherwise, the default behavior for an undefined index is to
// append block to the end of the rootClientId context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Related:

const { index = subState.length } = action;
return {
...state,
...mappedBlocks,
[ rootClientId ]: insertAt( subState, mappedBlocks[ rootClientId ], index ),
};

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Feel free to merge when tests pass 👍

@youknowriad youknowriad added this to the 4.2 milestone Oct 26, 2018
@aduth aduth merged commit 306cee8 into master Oct 26, 2018
@aduth aduth deleted the update/flatten-inserter-props branch October 26, 2018 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants