Edit Post: Select blocks only once multiple verified by aduth · Pull Request #12477 · WordPress/gutenberg

aduth · 2018-11-30T17:18:42Z

Related: #5031

This pull request seeks to optimize the multiple-use validation to avoid calling the getBlocks selector until after verifying that a non-multiple-supporting block is present in the post. This should help alleviate some performance bottleneck for a currently-underperforming selector (getBlocks), particularly when the the condition will not apply for the vast majority of blocks.

This is effectively a refactoring to reorganize code to only assign a variable value at a point where it is certain to be relevant for use by the remainder of the function. As an aside, it would be very interesting to try to develop a lint rule to verify that a variable is used before the next return statement.

Testing instructions:

Repeat testing instructions from #5031

Add a console.count at the point the getBlocks function is called, and verify that it is not called while authoring paragraph content (unlike before).

jorgefilipecosta

Looks good in my tests, as expected no behavior changes were noticed 👍
As referred, this would for sure be an excellent lint rule. Maybe it was already implemented by someone and is available.
The end 2 end tests fail seems totally unrelated and an intermittent problem, I restarted them.

aduth · 2018-11-30T19:32:08Z

As referred, this would for sure be an excellent lint rule. Maybe it was already implemented by someone and is available.

I looked around a bit, and unfortunately couldn't find much. I started working on a rule, and found a few edge cases, mostly stylistic:

We often destructure-assign a variable which may not be used before the return (example)
- Solution Exempt destructuring?
Sometimes it's not a "big deal" to assign a variable (example)
- Solution: Limit to expensive operations; namely when initialized to CallExpression (a function call)?

Here's my work-in-progress:

https://astexplorer.net/#/gist/dc29d9096eac1664d8eb287139fa55ac/94f70e52c05456e1092f3109e5f882d60b68f076

It finds a lot of very interesting faults!

Full report

/Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/bin/packages/watch.js 40:5 error Assigned variable `filePath` needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/block-library/src/cover/index.js 329:5 error Assigned variable classes needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/block-library/src/image/edit.js 419:4 error Assigned variable imageSizeOptions needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/blocks/src/api/factory.js 125:3 error Assigned variable sourceBlock needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 131:3 error Assigned variable sourceBlock needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/blocks/src/api/serializer.js 271:4 error Assigned variable saveAttributes needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/core-data/src/queried-data/selectors.js 33:3 error Assigned variable page needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 33:3 error Assigned variable perPage needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 38:3 error Assigned variable page needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 38:3 error Assigned variable perPage needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/dom/src/dom.js 395:4 error Assigned variable selection needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 399:3 error Assigned variable selection needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/edit-post/src/hooks/validate-multiple-use/index.js 42:4 error Assigned variable blocks needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/components/block-drop-zone/index.js 101:4 error Assigned variable srcRootClientId needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 101:4 error Assigned variable srcIndex needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/components/block-switcher/index.js 61:5 error Assigned variable blockType needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/components/post-saved-state/index.js 69:4 error Assigned variable hasPublishAction needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 78:4 error Assigned variable hasPublishAction needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 82:4 error Assigned variable hasPublishAction needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 86:4 error Assigned variable hasPublishAction needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/components/rich-text/index.js 246:5 error Assigned variable item needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 621:4 error Assigned variable record needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages 179:4 error Assigned variable pos needlessly assigned 189:4 error Assigned variable pos needlessly assigned 236:4 error Assigned variable pos needlessly assigned 242:4 error Assigned variable pos needlessly assigned 304:4 error Assigned variable pos needlessly assigned 323:4 error Assigned variable pos needlessly assigned 330:4 error Assigned variable pos needlessly assigned 335:4 error Assigned variable pos needlessly assigned 347:4 error Assigned variable pos needlessly assigned 367:4 error Assigned variable pos needlessly assigned 372:4 error Assigned variable pos needlessly assigned 372:4 error Assigned variable supports needlessly assigned 378:4 error Assigned variable pos needlessly assigned 378:4 error Assigned variable supports needlessly assigned 378:4 error Assigned variable style needlessly assigned 397:4 error Assigned variable pos needlessly assigned 401:4 error Assigned variable pos needlessly assigned 407:4 error Assigned variable pos needlessly assigned 407:4 error Assigned variable style needlessly assigned 425:4 error Assigned variable pos needlessly assigned 430:4 error Assigned variable pos needlessly assigned 430:4 error Assigned variable media needlessly assigned 436:4 error Assigned variable media needlessly assigned 436:4 error Assigned variable style needlessly assigned 436:4 error Assigned variable pos needlessly assigned 454:4 error Assigned variable pos needlessly assigned 472:4 error Assigned variable pos needlessly assigned 478:4 error Assigned variable pos needlessly assigned 491:4 error Assigned variable pos needlessly assigned 509:4 error Assigned variable pos needlessly assigned 516:4 error Assigned variable pos needlessly assigned 516:4 error Assigned variable vendor needlessly assigned 516:4 error Assigned variable doc needlessly assigned 522:4 error Assigned variable pos needlessly assigned 522:4 error Assigned variable vendor needlessly assigned 522:4 error Assigned variable doc needlessly assigned 522:4 error Assigned variable style needlessly assigned 541:4 error Assigned variable pos needlessly assigned 545:4 error Assigned variable pos needlessly assigned 558:4 error Assigned variable pos needlessly assigned 595:5 error Assigned variable pos needlessly assigned 634:4 error Assigned variable pos needlessly assigned /editor/src/editor-styles/ast/parse.js rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/editor-styles/ast/stringify/identity.js 250:3 error Assigned variable indent needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/store/effects.js 167:4 error Assigned variable blockB needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/store/effects/posts.js 58:3 error Assigned variable post needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return

/Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/packages/editor/src/store/selectors.js 298:4 error Assigned variable edits needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return 355:3 error Assigned variable password needlessly assigned rule-no-unused-vars-before-return/no-unused-vars-before-return

jorgefilipecosta · 2018-11-30T20:24:52Z

Nice exploration 👍

We often destructure-assign a variable which may not be used before the return (example)
Solution Exempt destructuring?

For the destructuring case, I guess an error should be thrown if none of the variables destructured is used before the return if at least one is used I think it is fine.

Solution: Limit to expensive operations; namely when initialized to CallExpression (a function call)?

Yes, I think only limiting to function calls makes sense.

Edit Post: Select blocks only once multiple verified

817bf05

aduth added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Nov 30, 2018

jorgefilipecosta approved these changes Nov 30, 2018

View reviewed changes

aduth added this to the 4.7 milestone Nov 30, 2018

aduth merged commit 459984e into master Nov 30, 2018

aduth deleted the update/validate-multiple-fast-path-return branch November 30, 2018 19:48

aduth mentioned this pull request Dec 4, 2018

Performance: Improve the initial loading time and the feeling fo smoothness while typing in blocks. #11782

Closed

This was referenced Dec 12, 2018

Performance: Move unused variable initializations after early return #12827

Merged

eslint-plugin: Add rule no-unused-vars-before-return #12828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit Post: Select blocks only once multiple verified#12477

Edit Post: Select blocks only once multiple verified#12477
aduth merged 1 commit intomasterfrom
update/validate-multiple-fast-path-return

aduth commented Nov 30, 2018 •

edited

Loading

Uh oh!

jorgefilipecosta left a comment

Uh oh!

aduth commented Nov 30, 2018

Uh oh!

jorgefilipecosta commented Nov 30, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

aduth commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

aduth commented Nov 30, 2018

Uh oh!

jorgefilipecosta commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aduth commented Nov 30, 2018 •

edited

Loading

jorgefilipecosta commented Nov 30, 2018 •

edited

Loading