-
Notifications
You must be signed in to change notification settings - Fork 613
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
fix(#1167): function calls prefixed with void
together with the optional chain (?.) are skipped
#1178
Conversation
… the optional chain (?.) are skipped
This update includes the NodePath<BabelNodeOptionalCallExpression> as part of the paths considered unsafe.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
=======================================
Coverage 83.31% 83.31%
=======================================
Files 207 207
Lines 10633 10633
Branches 2642 2642
=======================================
Hits 8859 8859
Misses 1774 1774 ☔ View full report in Codecov by Sentry. |
void
together with the optional chain (?.) are skippedvoid
together with the optional chain (?.) are skipped on production (dev: false
).
void
together with the optional chain (?.) are skipped on production (dev: false
).void
together with the optional chain (?.) are skipped
@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This is a great catch @Gamote - thanks for the fix! (To follow up, we ought to invert this logic so that nodes are default unsafe.) |
@robhogan Is there anything specific that I can do to fix this? |
Thanks, but nothing's broken, just a couple of flaky tests. We're just waiting for internal review at the moment, shouldn't be long. |
Ah alright. Thank you for the information. |
…ional chain (?.) are skipped (#1178) Summary: Fixes: #1167 `constant-folding-plugin` replaces this call `void foo?.();` with `undefined;`. This is because the `evaluate()` function marks the optional call expressions as safe, making the `UnaryExpression` visitor think it is safe to replace it with the `value` returned by the `evaluate()`, in this case `undefined`. This PR makes the `evaluate()` function mark the optional call expressions as unsafe. ``` * **[Fix]**: `constant-folding-plugin`: Don't fold optional function calls (`foo.?()`). ``` Pull Request resolved: #1178 Test Plan: I have added 2 tests to cover these changes: - does not transform optional chained call into `undefined` - does not transform `void` prefixed optional chained call into `undefined` Reviewed By: GijsWeterings Differential Revision: D52780448 Pulled By: robhogan fbshipit-source-id: c84288adc1fec6c2e875fd766c5a6b0997a36c62
…ional chain (?.) are skipped (#1178) Summary: Fixes: #1167 `constant-folding-plugin` replaces this call `void foo?.();` with `undefined;`. This is because the `evaluate()` function marks the optional call expressions as safe, making the `UnaryExpression` visitor think it is safe to replace it with the `value` returned by the `evaluate()`, in this case `undefined`. This PR makes the `evaluate()` function mark the optional call expressions as unsafe. ``` * **[Fix]**: `constant-folding-plugin`: Don't fold optional function calls (`foo.?()`). ``` Pull Request resolved: #1178 Test Plan: I have added 2 tests to cover these changes: - does not transform optional chained call into `undefined` - does not transform `void` prefixed optional chained call into `undefined` Reviewed By: GijsWeterings Differential Revision: D52780448 Pulled By: robhogan fbshipit-source-id: c84288adc1fec6c2e875fd766c5a6b0997a36c62
Summary
Fixes: #1167
constant-folding-plugin
replaces this callvoid foo?.();
withundefined;
.This is because the
evaluate()
function marks the optional call expressions as safe, making theUnaryExpression
visitor think it is safe to replace it with thevalue
returned by theevaluate()
, in this caseundefined
.This PR makes the
evaluate()
function mark the optional call expressions as unsafe.Changelog:
Test plan
I have added 2 tests to cover these changes:
undefined
void
prefixed optional chained call intoundefined