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

[Bug] ESBuild minify results in ReferenceError: o is not defined for a const defined in an if block #3125

Closed
sarahfank opened this issue May 19, 2023 · 5 comments

Comments

@sarahfank
Copy link

sarahfank commented May 19, 2023

Description

A const is defined within an if block and then used inside a function that is also defined in the if block. After minification when running in production, the value of the const is not being replaced properly in the function and is resulting in ReferenceError: o is not defined.

Reproduce

To reproduce this, create a new vite project unsing npm create vite@latest and replace the contents of main.ts with the following:

function main() {
  const is_visible = true;
  if (is_visible) {
    const MY_CONST = 20;

    console.log('above function call', MY_CONST);   // this works

    function myFunction () {
      console.log(MY_CONST);    // o is not defined
    }

    myFunction();
  }
}

main();

When you build and serve the dist folder and open in the browser, you will see ReferenceError: o is not defined.

@hyrious
Copy link

hyrious commented May 19, 2023

This issue involves a rollup tree shaking step, here's the reproduction steps without using vite: repl link

// main.ts = same as provided above

$ esbuild main.ts
function main() {
  const is_visible = true;
  if (is_visible) {
    let myFunction2 = function() {
      console.log(MY_CONST);
    };
    var myFunction = myFunction2;  // <- not used, will be shaked by rollup
    const MY_CONST = 20;
    console.log("above function call", MY_CONST);
    myFunction2();
  }
}
main();

$ esbuild main.ts | rollup
function main() {
  {
    let myFunction2 = function() {
      console.log(MY_CONST);  // <- constant folding bug!
    };
    const MY_CONST = 20;
    console.log("above function call", MY_CONST);
    myFunction2();
  }
}
main();

$ esbuild main.ts | rollup | esbuild --minify-syntax
function main() {
  {
    let myFunction2 = function() {
      console.log(MY_CONST);  // <- reference error
    };
    console.log("above function call", 20), myFunction2();
  }
}
main();

The steps corresponds to how vite build works:

  1. A rollup() at the outside..
  2. ..with an esbuild plugin that transpiles ts to js.
  3. Do esbuild --minify to the result of 1.

@DragonLiParadise
Copy link

I have also encountered a similar issue. Could you please let me know how you resolved it?

@hyrious
Copy link

hyrious commented May 22, 2023

@DragonLiParadise The issue is because esbuild is simulating the variable hoisting of function names and there's a bug in constant folding when a const name was used lexically before its difinition. So you could just don't write function body in arbitrary blocks (if-blocks, for-blocks, etc.)

@jluxenberg
Copy link

@evanw Thanks for your work on esbuild; it is saving us countless hours each day! ❤️

Spent a bit of time trying to fix this issue in js_parser.go but couldn't solve it.

I have a a few failing tests which illustrate the problem:
#3150

Hope these are helpful; interested to see what the fix ends up being.

@dylan-conway
Copy link

A possible fix for this would be:
js_parser:8210

if _, ok := p.constValues[id.Ref]; ok && p.symbols[id.Ref.InnerIndex].UseCountEstimate == 0 {

UseCountEstimate should decrement to 0 if all the const references are substituted in handleIdentifier. This change would avoid removing the declaration if one or more references aren't substituted.

@evanw what do you think?

@evanw evanw closed this as completed in 25a3963 Jun 20, 2023
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

5 participants