Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/babel-plugin-proposal-optional-chaining/src/index.js
Expand Up @@ -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];

Expand Down Expand Up @@ -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();
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should requeue replacementPath and not parentPath, since they are not always the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

}
},
},
Expand Down
@@ -0,0 +1,2 @@
const a = 1;
const b = () => a?.b?.c!.d;
@@ -0,0 +1,3 @@
{
"plugins": ["transform-typescript", "proposal-optional-chaining"]
}
@@ -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;
};