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

[metro-transform-plugins] Function calls prefixed with void together with optional chain (?.) are skipped on production (dev: false). #1167

Closed
Gamote opened this issue Jan 6, 2024 · 1 comment

Comments

@Gamote
Copy link

Gamote commented Jan 6, 2024

As the title suggests, today we found out that the constant-folding-plugin replaces this call void foo?.(); with undefined; on production (dev: false).

We found this issue while using Expo SDK 50 in production mode. In development mode works just fine because the constant-folding-plugin is skipped. For more information and a MRE please see this issue expo/expo#26276.

After some investigation we found out that we can fix this by adding OptionalCallExpression: unsafe in the evaluate method of the plugin. I am not 100% that this is the way to go as I don't have a lot of experience with Babel plugins and I might miss important implications. Please let me know if you have some guidance and I will be more than happy to proceed with a PR.

Here is the diff that solved my problem:

diff --git a/node_modules/metro-transform-plugins/src/constant-folding-plugin.js b/node_modules/metro-transform-plugins/src/constant-folding-plugin.js
index d0dd8ac..f1e96c6 100644
--- a/node_modules/metro-transform-plugins/src/constant-folding-plugin.js
+++ b/node_modules/metro-transform-plugins/src/constant-folding-plugin.js
@@ -27,8 +27,15 @@ function constantFoldingPlugin(context) {
     };
     path.traverse(
       {
-        CallExpression: unsafe,
         AssignmentExpression: unsafe,
+        CallExpression: unsafe,
+        /**
+         * This will mark `foo?.()` as unsafe, so it is not replaced with `undefined` down the line.
+         *
+         * We saw this case in the wild, where the unary expression `void foo?.()` was replaced with `undefined`
+         * resulting in the expression call being skipped.
+         */
+        OptionalCallExpression: unsafe,
       },
       state
     );
@Gamote
Copy link
Author

Gamote commented Jan 13, 2024

I have started a PR, hopefully it is the right direction: #1178.

@Gamote Gamote changed the title [metro-transform-plugins] Function calls prefixed with void together with optional chain (?.) are skipped [metro-transform-plugins] Function calls prefixed with void together with optional chain (?.) are skipped on production (dev: false). Jan 13, 2024
robhogan pushed a commit that referenced this issue Jan 29, 2024
…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
robhogan pushed a commit that referenced this issue Jan 30, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant