Performance: Move unused variable initializations after early return #12827
Conversation
aduth
force-pushed
the
perf/move-unused-declarations
branch
from
d893222
to
cebe585
Compare
| @@ -1,3 +1,5 @@ | |||
| /* eslint-disable @wordpress/no-unused-vars-before-return */ | |||
There was a problem hiding this comment.
Maybe we should update it. I just wasn't sure if we wanted to commit to a fork.
There was a problem hiding this comment.
If this is a fork, then I think it's fine to leave it as is.
| @@ -1,3 +1,5 @@ | |||
| /* eslint-disable @wordpress/no-unused-vars-before-return */ | |||
There was a problem hiding this comment.
So this is the code which errors:
Compiler.prototype.rule = function( node ) {
const indent = this.indent();
const decls = node.declarations;
if ( ! decls.length ) {
return '';
}Should we disable this check for objects somehow for the first iteration?
There was a problem hiding this comment.
Well, it's technically still wrong, but it raises an interesting point of whether there's mutation occurring in the this.indent() which needs to occur even if there's an early return. It seems like an edge case that we'd have (a) mutation, which (b) returns a value, and (c) needs to occur before the return.
There was a problem hiding this comment.
Yes, a lot of ifs here. It's hard to tell whether a given method mutates. Actually, this might be the case also in other places where you use regular function calls which modify some external variable.
There was a problem hiding this comment.
If it's obvious enough to a developer that they need the mutation to occur, I'm perfectly fine with having one-off rule disabling (our "Disable reason" pattern).
There was a problem hiding this comment.
Thinking more about it, the rule is good, if someone wants to mutate, they can disable as you pointed out. 👍
| const saveContent = getBlockContent( block ); | ||
| const saveAttributes = getCommentAttributes( blockType, block.attributes ); | ||
|
|
||
| switch ( blockName ) { | ||
| case getFreeformContentHandlerName(): | ||
| case getUnregisteredTypeHandlerName(): | ||
| return saveContent; | ||
|
|
||
| default: |
There was a problem hiding this comment.
Maybe we should add a block area here { } because to clarify that these consts are local to this default case?
There was a problem hiding this comment.
Definitely yes, because otherwise that can lead to some weird bugs/unexpected scoping.
We had an ESLint rule for this I think on the Moz Add-ons site… but I don't remember what it was 🤷♂️
aduth
force-pushed
the
perf/move-unused-declarations
branch
from
cebe585
to
e016b15
Compare
| const saveContent = getBlockContent( block ); | ||
| const saveAttributes = getCommentAttributes( blockType, block.attributes ); | ||
|
|
||
| switch ( blockName ) { | ||
| case getFreeformContentHandlerName(): | ||
| case getUnregisteredTypeHandlerName(): | ||
| return saveContent; | ||
|
|
||
| default: |
There was a problem hiding this comment.
Definitely yes, because otherwise that can lead to some weird bugs/unexpected scoping.
We had an ESLint rule for this I think on the Moz Add-ons site… but I don't remember what it was 🤷♂️
youknowriad
force-pushed
the
perf/move-unused-declarations
branch
from
770c000
to
097baba
Compare
|
Tests are failing badly here, not sure why. |
…12827) * Performance: Move unused variable initializations after early return * Small tweak about JS var scoping * Fix block switcher
Related: #12477
Blocks #12828
refs #11782
This pull request seeks to optimize written code by order of assignment. It can be considered a stylistic change, since the logic has not been revised; but it is one which has a potential performance impact. These issues were surfaced by the new ESLint rule introduced in #12828, where the assignments may be wasteful in that there are
returnpaths which occur earlier than the first reference to the assigned value.Testing instructions:
Verify there are no regressions in impacted code.