-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Performance: Move unused variable initializations after early return #12827
Conversation
d893222
to
cebe585
Compare
@@ -1,3 +1,5 @@ | |||
/* eslint-disable @wordpress/no-unused-vars-before-return */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you gave up here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about it, the rule is good, if someone wants to mutate, they can disable as you pointed out. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how to fix Travis, I removed all caches, re-triggered build a few times. It was rebased with master
🤷♂️
The code improvements themselves look great. We should land it as soon as possible.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤷♂️
cebe585
to
e016b15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be wrapping the default
block with { }
to set the variable scope. Unless this is critical to go out in 4.8, in which case feel free to dismiss my review 😉
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤷♂️
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
return
paths which occur earlier than the first reference to the assigned value.Testing instructions:
Verify there are no regressions in impacted code.