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

avoid var declaration overwritten #2085

Closed
wants to merge 2 commits into from

Conversation

magic-akari
Copy link
Contributor

@magic-akari magic-akari commented Mar 5, 2022

For the following input code

console.log(1, f);

function f() {
  x();
}

console.log(2, f);

var f = 1;

console.log(3, f);

function f() {
  y();
}

console.log(4, f);

var f = 2;

console.log(5, f);

The following output will be generated

1 ƒ f() {
  y();
}
2 ƒ f() {
  y();
}
3 1
4 1
5 2

This means that input is equivalent to the following code

var f;
function f() {
  x();
}
function f() {
  y();
}

console.log(1, f);
console.log(2, f);

f = 1;

console.log(3, f);
console.log(4, f);

f = 2;

console.log(5, f);

Functions can always be safely overwritten, whereas var declaration is not.

This PR tries to preserve the var declaration.

@magic-akari magic-akari marked this pull request as draft March 6, 2022 08:54
@magic-akari magic-akari force-pushed the fix/issue-2080 branch 2 times, most recently from 47802b7 to 6ee63c2 Compare March 13, 2022 13:49
- fix evanw#2080 regression introduced in 4fa3d7a
@magic-akari magic-akari marked this pull request as ready for review March 13, 2022 17:06
@magic-akari
Copy link
Contributor Author

It means we should keep all the vars and the last functions.
The equivalent code is as follows.

console.log(1, f);

// Safely delete
// function f() {
//   x();
// }

console.log(2, f);

var f = 1;

console.log(3, f);

// It should be kept since hoisting
function f() {
  y();
}

console.log(4, f);

var f = 2;

console.log(5, f);

@evanw evanw closed this in ea3b3d3 Mar 13, 2022
@evanw
Copy link
Owner

evanw commented Mar 13, 2022

Thanks for the PR! When I took a look at this, the bug had a simpler fix that also addressed the underlying issue, so I decided to make that change instead.

zhusjfaker pushed a commit to speedy-js/esbuild that referenced this pull request Mar 28, 2022
hardfist pushed a commit to speedy-js/esbuild that referenced this pull request Mar 28, 2022
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

Successfully merging this pull request may close these issues.

Regression in v0.14.x
2 participants