-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't look up variables past hoisting scope #2042
base: main
Are you sure you want to change the base?
Conversation
d54a263
to
cf4245a
Compare
} | ||
|
||
func (r *NumberRenamer) assignName(scope *numberScope, ref js_ast.Ref) { | ||
func (r *NumberRenamer) assignName(scope *numberScope, ref js_ast.Ref, shallowLookup bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to variable name suggestions 😂
p.recordDeclaredSymbol(fn.Name.Ref) | ||
} | ||
|
||
p.pushScopeForVisitPass(js_ast.ScopeFunctionArgs, scopeLoc) | ||
if fn.Name != nil && isExpr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct scope, but let me know if I should move it to function body instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to verify that it is indeed the correct scope.
cf4245a
to
cb1c3a0
Compare
default_suite.expectBundled(t, bundled{ | ||
files: map[string]string{ | ||
"/entry.js": ` | ||
export const a = function a(b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few tests. Let me know if you'd like to see more of them.
cb1c3a0
to
2ce7687
Compare
(This is actually still me, above is my work account) |
When we are renaming variables - we don't need to check for the conflicts past the closest scope that stops the hoisting. Additionally, function expression's name should be put into function's scope, and not outside of the function. Fix: evanw#1147
2ce7687
to
38ff6e1
Compare
This PR introduces name collisions into the bundler. Here's an example, which should print import { bar as baz } from 'data:text/javascript,export let bar = 123'
function foo(bar) {
return bar + baz
}
console.log(foo(321)) Bundling this with the latest release of esbuild results in the following, which prints (() => {
// <data:text/javascript,export let bar = 123>
var bar = 123;
// example.ts
function foo(bar2) {
return bar2 + bar;
}
console.log(foo(321));
})(); Bundling this with your PR gives this, which prints (() => {
// <data:text/javascript,export let bar = 123>
var bar = 123;
// example.ts
function foo(bar) {
return bar + bar;
}
console.log(foo(321));
})(); |
Sorry about that. Let me see... |
4ffe1da
to
7257144
Compare
Alright, it took me awhile to find a way, but I think I got something far enough to request additional feedback @evanw . The idea is to mark symbols post merge with WasMerged flag and place them in a It is possible to drop WasMerged flag, but at the cost of introducing extraneous renames. As an example:
I think this flag is very much needed because it tells that the symbol is used outside of the source where it was defined. Maybe I should rename flag to UsedExternally? |
I merged master into this PR to give it a test on some codebases; I found this case to be one that is also not handled. // index.ts
import * as mod from "./mod";
export function doSomething() {
return foo();
function foo() {
return mod.foo();
}
}
// mod.ts
export function foo() {} The correct output would be something like: // project/mod.ts
function foo() {
}
// project/index.ts
function doSomething() {
return foo2();
function foo2() {
return foo();
}
} However, this PR changes it to: // project/mod.ts
function foo() {
}
// project/index.ts
function doSomething() {
return foo();
function foo() {
return foo();
}
} That being said, I really like the affect this PR has on my outputs; it gets rid of a load of renames, mainly due to parameters not being renamed (since they are in a different scope entirely). |
@evanw please take another look? |
When we are renaming variables - we don't need to check for the
conflicts past the closest scope that stops the hoisting. Additionally,
function expression's name should be put into function's scope, and not
outside of the function.
Fix: #1147
# CaveatI might need some guidance with regards togenerateTempRef
andgenerateTopLevelTempRef
. They rely on the current behavior of the parser and it breaks tests for private properties because of the name clash. Given this the PR is in a draft state, even though all other tests pass.cc @evanw