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

Constant folding breaks some already minified code #291

Open
chpill opened this issue Oct 12, 2018 · 2 comments
Open

Constant folding breaks some already minified code #291

chpill opened this issue Oct 12, 2018 · 2 comments

Comments

@chpill
Copy link

chpill commented Oct 12, 2018

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

See https://github.com/chpill/repro-bug-metro

The index.js file contains JS compiled from Clojurescript and JS optimized by google closure compiler. The minimalist app boots without issue when __DEV__ == false or when minify == true.
But when __DEV == false && minify == true, the app crashes with a JS error because of an undefined variable (ReferenceError: $G__1978__22$$ is not defined).

It turns out the issue comes from the constant folding optimization when applied on our already optimized/minified index.js

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.

To reproduce the bug:

  • git clone https://github.com/chpill/repro-bug-metro
  • react-native start --reset-cache
  • react-native run-android then check that the app boots (you should see a green screen)
  • Shake the device and go to "Dev settings", adjust options so that __DEV == false && minify == true and reload
  • You should now see the exception ReferenceError: $G__1978__22$$ is not defined

To see that the problems comes from constant folding on our index.js:

if (!options.dev) {
  if (filename === "index.js")
    console.log("======= CUSTOM PATCH @chpill ======== IGNORING CONSTANT FOLDING FOR", filename);
  else
    plugins.push([constantFoldingPlugin, opts]);

  plugins.push([inlinePlugin, opts]);
}
  • stop the running packager if necessary, and launch react-native start --reset-cache
  • reload the app
  • Make sure you see the custom console log statement being issued while the packager is bundling the production minified artefact
  • You should now see the application booting without issue.

What is the expected behavior?

The constant folding optimization should not produce invalid JS code, or there should be a way to opt out of it for specific files.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

{
  "metro": "^0.48.1",
  "metro-babel-register": "^0.48.1",
  "metro-core": "^0.48.1",
  "metro-memory-fs": "^0.48.1"
}

yarn: 1.7.0
node: v8.11.3

OS: Linux 4.18.0-1-amd64
@raspasov
Copy link

I confirm this is a problem. I ran into it as well with minified Google Closure code.

Thanks @chpill for the patch, it worked as a temp solution.

@leops
Copy link
Contributor

leops commented Mar 4, 2021

I can confirm this issue seems to still exist in the latest version of Metro, I did not hit this with minified code but with hand written library code so I managed to reduce it to a simple test case, and it seems to be caused by a function declaring a variable shadowing its own name:

function foo() {
    let foo;
}

// Side Effect
console.log(foo);

With dev=true the module is outputted correctly, however with dev=false only the console.log statement is emitted and the function is removed. I used console.log as a side-effect here but the bug is also reproducible when the function is exported. This was discovered on React Native 0.63.4 (Metro 0.58.0) and tested on Metro 0.65.2.

facebook-github-bot pushed a commit that referenced this issue Mar 25, 2021
Summary:
**Summary**

This is a possibly incomplete fix for #291, it corrects at least one issue with name shadowing but there might be other problems present in the very large code payload presented in the original issue. The exact bug fixed here happens when a function declares an unused variable shadowing its own name:

```js
export function foo() {
    let foo;
}
```

The current code checks whether that the binding corresponding to the function is referenced in the scope of the FunctionDeclaration node, however in Babel that scope is situated inside the body of the function and not in the outer program. Since the binding is retrieved by name, Babel returns informations related to the inner, unreferenced variable and thus the entire function is removed. This fix changes the biding retrieval to read from the parent scope of the FunctionDefinition, which returns the correct binding.

**Test plan**

An additional test case has been added to verify the function remains in the final output.

Pull Request resolved: #643

Reviewed By: MichaReiser

Differential Revision: D27324443

Pulled By: motiz88

fbshipit-source-id: 30a9b7244846ad0ff4ba080c96e81dc6f582e5e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants