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
Requeue parent path after processing optionals #12389
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,9 @@ export default declare((api, options) => { | |
replacementPath = parentPath; | ||
isDeleteOperation = true; | ||
} | ||
|
||
let needsRequeue = false; | ||
|
||
for (let i = optionals.length - 1; i >= 0; i--) { | ||
const node = optionals[i]; | ||
|
||
|
@@ -237,6 +240,15 @@ export default declare((api, options) => { | |
replacementPath.get("alternate"), | ||
); | ||
} | ||
|
||
needsRequeue = true; | ||
} | ||
|
||
// TODO(bng): Continue investigating why changes in https://github.com/babel/babel/pull/12302 | ||
// now need this requeue (and also why this _specific_ requeue, | ||
// replacementPath.requeue() doesn't work). | ||
if (needsRequeue) { | ||
parentPath.requeue(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should requeue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my first thought too, but there's some non obvious thing going on, still digging further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh ok. Do you think that it's better to merge this PR since "it works", and then revisit for another release? Or should we wait? |
||
} | ||
}, | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
const a = 1; | ||
const b = () => a?.b?.c!.d; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"plugins": ["transform-typescript", "proposal-optional-chaining"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
const a = 1; | ||
|
||
const b = () => { | ||
var _a$b; | ||
|
||
return a === null || a === void 0 ? void 0 : (_a$b = a.b) === null || _a$b === void 0 ? void 0 : _a$b.c.d; | ||
}; |
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.
Not a blocker, but we should figure out why the test passes without requeuing parent path on
@babel/traverse
7.12.3.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.
For sure, in the middle of digging actually
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.
It looks like: #12302, still digging