Fix VariableDeclaration visitors #6288
Conversation
lib/Parser.js
Outdated
| for(const declarator of declarators) { | ||
| if(statement.type !== "VariableDeclaration") | ||
| return; | ||
| const hookMap = statement.kind === "const" ? this.hooks.varDeclarationConst : |
There was a problem hiding this comment.
Here is the main change
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
There was a problem hiding this comment.
This looks fine to me. For most parser stuff, I feel good letting @sokra take a review as well. 👍 Nice work.
lib/Parser.js
Outdated
|
|
||
| prewalkVariableDeclarators(declarators) { | ||
| for(const declarator of declarators) { | ||
| if(statement.type !== "VariableDeclaration") |
There was a problem hiding this comment.
Why is this needed, shouldn't the function only be called for VariableDeclarations?
There was a problem hiding this comment.
Well, technically yes but this is not how the parser uses it. walkVariableDeclaration is also called with ForStatement#init or For*Statement#left which also accepts identifiers or destructured patterns.
There was a problem hiding this comment.
hmm... could you move the "if" to these calls.
lib/Parser.js
Outdated
| walkVariableDeclarators(declarators) { | ||
| for(const declarator of declarators) { | ||
| walkVariableDeclaration(statement) { | ||
| if(statement.type !== "VariableDeclaration") |
There was a problem hiding this comment.
Why is this needed, shouldn't the function only be called for VariableDeclarations?
1739bc4 to
e44ba30
Compare
|
@ooflorent Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
Thanks |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
no
If relevant, link to documentation update:
n/a
Summary
Prior this change, the
VariableDeclarationnodes were not properly walked. Thekindproperty was read on theVariableDeclaratorinstead of the parent node.Sorry for the diff but git decided to go this way.
Does this PR introduce a breaking change?
no