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.

Sign up for GitHub

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

Apply defines before injected global variables #1261

Merged
merged 1 commit into from
May 18, 2021
Merged

Apply defines before injected global variables #1261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions internal/bundler/bundler_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3627,13 +3627,25 @@ func TestInject(t *testing.T) {
return &js_ast.EIdentifier{Ref: args.FindSymbol(args.Loc, "replace")}
},
},
"obj.defined": {
DefineFunc: func(args config.DefineArgs) js_ast.E {
return &js_ast.EString{Value: js_lexer.StringToUTF16("defined")}
},
},
"injectedAndDefined": {
DefineFunc: func(args config.DefineArgs) js_ast.E {
return &js_ast.EString{Value: js_lexer.StringToUTF16("should be used")}
},
},
})
default_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
let sideEffects = console.log('this should be renamed')
let collide = 123
console.log(obj.prop)
console.log(obj.defined)
console.log(injectedAndDefined)
console.log(chain.prop.test)
console.log(collide)
console.log(re_export)
Expand All @@ -3642,6 +3654,7 @@ func TestInject(t *testing.T) {
export let obj = {}
export let sideEffects = console.log('side effects')
export let noSideEffects = /* @__PURE__ */ console.log('side effects')
export let injectedAndDefined = 'should not be used'
`,
"/node_modules/unused/index.js": `
console.log('This is unused but still has side effects')
Expand Down Expand Up @@ -3694,13 +3707,25 @@ func TestInjectNoBundle(t *testing.T) {
return &js_ast.EIdentifier{Ref: args.FindSymbol(args.Loc, "replace")}
},
},
"obj.defined": {
DefineFunc: func(args config.DefineArgs) js_ast.E {
return &js_ast.EString{Value: js_lexer.StringToUTF16("defined")}
},
},
"injectedAndDefined": {
DefineFunc: func(args config.DefineArgs) js_ast.E {
return &js_ast.EString{Value: js_lexer.StringToUTF16("should be used")}
},
},
})
default_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
let sideEffects = console.log('this should be renamed')
let collide = 123
console.log(obj.prop)
console.log(obj.defined)
console.log(injectedAndDefined)
console.log(chain.prop.test)
console.log(collide)
console.log(re_export)
Expand All @@ -3709,6 +3734,7 @@ func TestInjectNoBundle(t *testing.T) {
export let obj = {}
export let sideEffects = console.log('side effects')
export let noSideEffects = /* @__PURE__ */ console.log('side effects')
export let injectedAndDefined = 'should not be used'
`,
"/node_modules/unused/index.js": `
console.log('This is unused but still has side effects')
Expand Down
4 changes: 4 additions & 0 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,8 @@ var import_external_pkg = __toModule(require("external-pkg"));
var sideEffects2 = console.log("this should be renamed");
var collide = 123;
console.log(obj.prop);
console.log("defined");
console.log("should be used");
console.log(replace.test);
console.log(collide);
console.log(import_external_pkg.re_export);
Expand Down Expand Up @@ -1244,6 +1246,8 @@ import {re_export} from "external-pkg";
let sideEffects2 = console.log("this should be renamed");
let collide = 123;
console.log(obj.prop);
console.log("defined");
console.log("should be used");
console.log(replace.test);
console.log(collide);
console.log(re_export);
Expand Down
7 changes: 7 additions & 0 deletions internal/js_ast/js_ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,6 +1316,9 @@ const (
// Assigning to a "const" symbol will throw a TypeError at runtime
SymbolConst

// Injected symbols can be overridden by provided defines
SymbolInjected

// This annotates all other symbols that don't have special behavior.
SymbolOther
)
Expand Down Expand Up @@ -1355,6 +1358,10 @@ func (kind SymbolKind) IsFunction() bool {
return kind == SymbolHoistedFunction || kind == SymbolGeneratorOrAsyncFunction
}

func (kind SymbolKind) IsUnboundOrInjected() bool {
return kind == SymbolUnbound || kind == SymbolInjected
}

var InvalidRef Ref = Ref{^uint32(0), ^uint32(0)}

// Files are parsed in parallel for speed. We want to allow each parser to
Expand Down
16 changes: 10 additions & 6 deletions internal/js_parser/js_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9508,8 +9508,10 @@ func (p *parser) isDotDefineMatch(expr js_ast.Expr, parts []string) bool {
return false
}

// The last symbol must be unbound
return p.symbols[result.ref.InnerIndex].Kind == js_ast.SymbolUnbound
p.ignoreUsage(result.ref)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace an injected symbol with a define, we need to explicitly mark it as unused (as p.findSymbol has the effect of marking usage). Otherwise, the injected import won't be eliminated.

Calling p.ignoreUsage feels a bit hacky. I also suppose that ignoreUsage should be invoked closer to the actual replacement callsite rather than in isDotDefineMatch, which doesn't necessitate that a replacement actually occurs...


// The last symbol must be unbound or injected
return p.symbols[result.ref.InnerIndex].Kind.IsUnboundOrInjected()
}
}

Expand Down Expand Up @@ -10312,15 +10314,16 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
}
}

// Substitute user-specified defines for unbound symbols
if p.symbols[e.Ref.InnerIndex].Kind == js_ast.SymbolUnbound && !result.isInsideWithScope && e != p.deleteTarget {
// Substitute user-specified defines for unbound or injected symbols
if p.symbols[e.Ref.InnerIndex].Kind.IsUnboundOrInjected() && !result.isInsideWithScope && e != p.deleteTarget {
if data, ok := p.options.defines.IdentifierDefines[name]; ok {
if data.DefineFunc != nil {
new := p.valueForDefine(expr.Loc, in.assignTarget, isDeleteTarget, data.DefineFunc)

// Don't substitute an identifier for a non-identifier if this is an
// assignment target, since it'll cause a syntax error
if _, ok := new.Data.(*js_ast.EIdentifier); in.assignTarget == js_ast.AssignTargetNone || ok {
p.ignoreUsage(e.Ref)
return new, exprOut{}
}
}
Expand Down Expand Up @@ -13193,15 +13196,16 @@ func Parse(log logger.Log, source logger.Source, options Options) (result js_ast
exportsNoConflict := make([]string, 0, len(file.Exports))
symbols := make(map[string]js_ast.Ref)
if file.IsDefine {
ref := p.newSymbol(js_ast.SymbolOther, js_ast.GenerateNonUniqueNameFromPath(file.Path))
ref := p.newSymbol(js_ast.SymbolInjected, js_ast.GenerateNonUniqueNameFromPath(file.Path))

p.moduleScope.Generated = append(p.moduleScope.Generated, ref)
symbols["default"] = ref
exportsNoConflict = append(exportsNoConflict, "default")
p.injectedDefineSymbols = append(p.injectedDefineSymbols, ref)
} else {
for _, alias := range file.Exports {
if _, ok := p.moduleScope.Members[alias]; !ok {
ref := p.newSymbol(js_ast.SymbolOther, alias)
ref := p.newSymbol(js_ast.SymbolInjected, alias)
p.moduleScope.Members[alias] = js_ast.ScopeMember{Ref: ref}
symbols[alias] = ref
exportsNoConflict = append(exportsNoConflict, alias)
Expand Down