From a5288c6795e8639082b89625df8bfa954c967419 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Tue, 29 Dec 2020 03:40:24 -0800 Subject: [PATCH] fix #581, close #582: auto-inject object defines --- CHANGELOG.md | 6 +++++ internal/bundler/bundler.go | 45 ++++++++++++++++++++++++++++++++- internal/bundler/linker.go | 26 +++++++++++-------- internal/config/config.go | 9 +++++++ internal/config/globals.go | 5 ++-- internal/js_ast/js_ast.go | 4 +++ internal/js_parser/js_parser.go | 38 +++++++++++++++++++++------- pkg/api/api_impl.go | 37 ++++++++++++++++++++++----- scripts/js-api-tests.js | 26 +++++++++++++++++++ 9 files changed, 168 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21974b386c7..21289be251c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ This release fixes an edge case where files in ECMAScript module format that are converted to CommonJS format during bundling can generate exports to non-top-level symbols when code splitting is active. These files must be converted to CommonJS format if they are referenced by a `require()` call. When that happens, the symbols in that file are placed inside the CommonJS wrapper closure and are no longer top-level symbols. This means they should no longer be considered exportable for cross-chunk export generation due to code splitting. The result of this fix is that these cases no longer generate output files with module instantiation errors. +* Allow `--define` with array and object literals ([#581](https://github.com/evanw/esbuild/issues/581)) + + The `--define` feature allows you to replace identifiers such as `DEBUG` with literal expressions such as `false`. This is valuable because the substitution can then participate in constant folding and dead code elimination. For example, `if (DEBUG) { ... }` could become `if (false) { ... }` which would then be completely removed in minified builds. However, doing this with compound literal expressions such as array and object literals is an anti-pattern because it could easily result in many copies of the same object in the output file. + + This release adds support for array and object literals with `--define` anyway, but they work differently than other `--define` expressions. In this case a separate virtual file is created and configured to be injected into all files similar to how the `--inject` feature works. This means there is only at most one copy of the value in a given output file. However, these values do not participate in constant folding and dead code elimination, since the object can now potentially be mutated at run-time. + ## 0.8.26 * Ensure the current working directory remains unique per `startService()` call diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index 05be29bb6c9..ee4ff1a0711 100644 --- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -944,9 +944,51 @@ func (s *scanner) allocateSourceIndex(path logger.Path, kind cache.SourceIndexKi } func (s *scanner) preprocessInjectedFiles() { - injectedFiles := make([]config.InjectedFile, 0, len(s.options.InjectAbsPaths)) + injectedFiles := make([]config.InjectedFile, 0, len(s.options.InjectedDefines)+len(s.options.InjectAbsPaths)) duplicateInjectedFiles := make(map[string]bool) injectWaitGroup := sync.WaitGroup{} + + // These are virtual paths that are generated for compound "--define" values. + // They are special-cased and are not available for plugins to intercept. + for _, define := range s.options.InjectedDefines { + // These should be unique by construction so no need to check for collisions + visitedKey := logger.Path{Text: fmt.Sprintf("", define.Name)} + sourceIndex := s.allocateSourceIndex(visitedKey, cache.SourceIndexNormal) + s.visited[visitedKey] = sourceIndex + source := logger.Source{ + Index: sourceIndex, + KeyPath: visitedKey, + PrettyPath: s.res.PrettyPath(visitedKey), + IdentifierName: js_ast.EnsureValidIdentifier(visitedKey.Text), + } + + // The first "len(InjectedDefine)" injected files intentionally line up + // with the injected defines by index. The index will be used to import + // references to them in the parser. + injectedFiles = append(injectedFiles, config.InjectedFile{ + Path: visitedKey.Text, + SourceIndex: sourceIndex, + IsDefine: true, + }) + + // Generate the file inline here since it has already been parsed + expr := js_ast.Expr{Data: define.Data} + ast := js_parser.LazyExportAST(s.log, source, js_parser.OptionsFromConfig(&s.options), expr, "") + result := parseResult{ + ok: true, + file: file{ + source: source, + loader: config.LoaderJSON, + repr: &reprJS{ast: ast}, + ignoreIfUnused: true, + }, + } + + // Append to the channel on a goroutine in case it blocks due to capacity + s.remaining++ + go func() { s.resultChannel <- result }() + } + for _, absPath := range s.options.InjectAbsPaths { prettyPath := s.res.PrettyPath(logger.Path{Text: absPath, Namespace: "file"}) lowerAbsPath := lowerCaseAbsPathForWindows(absPath) @@ -976,6 +1018,7 @@ func (s *scanner) preprocessInjectedFiles() { injectWaitGroup.Done() }(i, prettyPath, resolveResult) } + injectWaitGroup.Wait() s.options.InjectedFiles = injectedFiles } diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index 231bf439fb4..514bc9d1841 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -413,8 +413,7 @@ func newLinkerContext( repr.meta = fileMeta{ cjsStyleExports: repr.ast.HasCommonJSFeatures() || (options.Mode == config.ModeBundle && repr.ast.ModuleScope.ContainsDirectEval) || - (repr.ast.HasLazyExport && (c.options.Mode == config.ModePassThrough || - (c.options.Mode == config.ModeConvertFormat && !c.options.OutputFormat.KeepES6ImportExportSyntax()))), + (repr.ast.HasLazyExport && c.options.Mode == config.ModeConvertFormat && !c.options.OutputFormat.KeepES6ImportExportSyntax()), partMeta: make([]partMeta, len(repr.ast.Parts)), resolvedExports: resolvedExports, isProbablyTypeScriptType: make(map[js_ast.Ref]bool), @@ -451,14 +450,21 @@ func newLinkerContext( file := &c.files[sourceIndex] file.isEntryPoint = true - // Entry points with ES6 exports must generate an exports object when - // targeting non-ES6 formats. Note that the IIFE format only needs this - // when the global name is present, since that's the only way the exports - // can actually be observed externally. - if repr, ok := file.repr.(*reprJS); ok && repr.ast.HasES6Exports && (options.OutputFormat == config.FormatCommonJS || - (options.OutputFormat == config.FormatIIFE && len(options.GlobalName) > 0)) { - repr.ast.UsesExportsRef = true - repr.meta.forceIncludeExportsForEntryPoint = true + if repr, ok := file.repr.(*reprJS); ok { + // Lazy exports default to CommonJS-style for the transform API + if repr.ast.HasLazyExport && c.options.Mode == config.ModePassThrough { + repr.meta.cjsStyleExports = true + } + + // Entry points with ES6 exports must generate an exports object when + // targeting non-ES6 formats. Note that the IIFE format only needs this + // when the global name is present, since that's the only way the exports + // can actually be observed externally. + if repr.ast.HasES6Exports && (options.OutputFormat == config.FormatCommonJS || + (options.OutputFormat == config.FormatIIFE && len(options.GlobalName) > 0)) { + repr.ast.UsesExportsRef = true + repr.meta.forceIncludeExportsForEntryPoint = true + } } } diff --git a/internal/config/config.go b/internal/config/config.go index b8723d08898..1d157e07261 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "sync" "github.com/evanw/esbuild/internal/compat" + "github.com/evanw/esbuild/internal/js_ast" "github.com/evanw/esbuild/internal/logger" ) @@ -207,6 +208,7 @@ type Options struct { OutputFormat Format PublicPath string InjectAbsPaths []string + InjectedDefines []InjectedDefine InjectedFiles []InjectedFile Banner string Footer string @@ -222,10 +224,17 @@ type Options struct { Stdin *StdinInfo } +type InjectedDefine struct { + Source logger.Source + Data js_ast.E + Name string +} + type InjectedFile struct { Path string SourceIndex uint32 Exports []string + IsDefine bool } var filterMutex sync.Mutex diff --git a/internal/config/globals.go b/internal/config/globals.go index b0127d3058e..b12db5c930c 100644 --- a/internal/config/globals.go +++ b/internal/config/globals.go @@ -113,8 +113,9 @@ var knownGlobals = [][]string{ } type DefineArgs struct { - Loc logger.Loc - FindSymbol func(logger.Loc, string) js_ast.Ref + Loc logger.Loc + FindSymbol func(logger.Loc, string) js_ast.Ref + SymbolForDefine func(int) js_ast.Ref } type DefineFunc func(DefineArgs) js_ast.E diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index 5b3c10ae804..e2571f99c99 100644 --- a/internal/js_ast/js_ast.go +++ b/internal/js_ast/js_ast.go @@ -1654,6 +1654,10 @@ func GenerateNonUniqueNameFromPath(path string) string { } } + return EnsureValidIdentifier(base) +} + +func EnsureValidIdentifier(base string) string { // Convert it to an ASCII identifier. Note: If you change this to a non-ASCII // identifier, you're going to potentially cause trouble with non-BMP code // points in target environments that don't support bracketed Unicode escapes. diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index a1fe1afad7e..56abfcb2427 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -57,6 +57,8 @@ type parser struct { importMetaRef js_ast.Ref promiseRef js_ast.Ref findSymbolHelper func(loc logger.Loc, name string) js_ast.Ref + symbolForDefineHelper func(int) js_ast.Ref + injectedDefineSymbols []js_ast.Ref symbolUses map[js_ast.Ref]js_ast.SymbolUse declaredSymbols []js_ast.DeclaredSymbol runtimeImports map[string]js_ast.Ref @@ -9987,8 +9989,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO func (p *parser) valueForDefine(loc logger.Loc, assignTarget js_ast.AssignTarget, isDeleteTarget bool, defineFunc config.DefineFunc) js_ast.Expr { expr := js_ast.Expr{Loc: loc, Data: defineFunc(config.DefineArgs{ - Loc: loc, - FindSymbol: p.findSymbolHelper, + Loc: loc, + FindSymbol: p.findSymbolHelper, + SymbolForDefine: p.symbolForDefineHelper, })} if id, ok := expr.Data.(*js_ast.EIdentifier); ok { return p.handleIdentifier(loc, assignTarget, isDeleteTarget, id) @@ -10976,7 +10979,16 @@ func newParser(log logger.Log, source logger.Source, lexer js_lexer.Lexer, optio namedExports: make(map[string]js_ast.NamedExport), } - p.findSymbolHelper = func(loc logger.Loc, name string) js_ast.Ref { return p.findSymbol(loc, name).ref } + p.findSymbolHelper = func(loc logger.Loc, name string) js_ast.Ref { + return p.findSymbol(loc, name).ref + } + + p.symbolForDefineHelper = func(index int) js_ast.Ref { + ref := p.injectedDefineSymbols[index] + p.recordUsage(ref) + return ref + } + p.pushScopeForParsePass(js_ast.ScopeEntry, logger.Loc{Start: locModuleScope}) return p @@ -11058,12 +11070,20 @@ func Parse(log logger.Log, source logger.Source, options Options) (result js_ast for _, file := range p.options.injectedFiles { exportsNoConflict := make([]string, 0, len(file.Exports)) symbols := make(map[string]js_ast.Ref) - for _, alias := range file.Exports { - if _, ok := p.moduleScope.Members[alias]; !ok { - ref := p.newSymbol(js_ast.SymbolOther, alias) - p.moduleScope.Members[alias] = js_ast.ScopeMember{Ref: ref} - symbols[alias] = ref - exportsNoConflict = append(exportsNoConflict, alias) + if file.IsDefine { + ref := p.newSymbol(js_ast.SymbolOther, 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) + p.moduleScope.Members[alias] = js_ast.ScopeMember{Ref: ref} + symbols[alias] = ref + exportsNoConflict = append(exportsNoConflict, alias) + } } } before = p.generateImportStmt(file.Path, exportsNoConflict, file.SourceIndex, before, symbols) diff --git a/pkg/api/api_impl.go b/pkg/api/api_impl.go index 7a44c3c204a..1dbaf7e3f83 100644 --- a/pkg/api/api_impl.go +++ b/pkg/api/api_impl.go @@ -316,12 +316,14 @@ func validateJSX(log logger.Log, text string, name string) []string { return parts } -func validateDefines(log logger.Log, defines map[string]string, pureFns []string) *config.ProcessedDefines { +func validateDefines(log logger.Log, defines map[string]string, pureFns []string) (*config.ProcessedDefines, []config.InjectedDefine) { if len(defines) == 0 && len(pureFns) == 0 { - return nil + return nil, nil } rawDefines := make(map[string]config.DefineData) + valueToInject := make(map[string]config.InjectedDefine) + var definesToInject []string for key, value := range defines { // The key must be a dot-separated identifier list @@ -359,9 +361,9 @@ func validateDefines(log logger.Log, defines map[string]string, pureFns []string continue } - // Only allow atoms for now var fn config.DefineFunc switch e := expr.Data.(type) { + // These values are inserted inline, and can participate in constant folding case *js_ast.ENull: fn = func(config.DefineArgs) js_ast.E { return &js_ast.ENull{} } case *js_ast.EBoolean: @@ -370,6 +372,13 @@ func validateDefines(log logger.Log, defines map[string]string, pureFns []string fn = func(config.DefineArgs) js_ast.E { return &js_ast.EString{Value: e.Value} } case *js_ast.ENumber: fn = func(config.DefineArgs) js_ast.E { return &js_ast.ENumber{Value: e.Value} } + + // These values are extracted into a shared symbol reference + case *js_ast.EArray, *js_ast.EObject: + definesToInject = append(definesToInject, key) + valueToInject[key] = config.InjectedDefine{Source: source, Data: e, Name: key} + continue + default: log.AddError(nil, logger.Loc{}, fmt.Sprintf("Invalid define value: %q", value)) continue @@ -378,6 +387,18 @@ func validateDefines(log logger.Log, defines map[string]string, pureFns []string rawDefines[key] = config.DefineData{DefineFunc: fn} } + // Sort injected defines for determinism, since the imports will be injected + // into every file in the order that we return them from this function + injectedDefines := make([]config.InjectedDefine, len(definesToInject)) + sort.Strings(definesToInject) + for i, key := range definesToInject { + index := i // Capture this for the closure below + injectedDefines[i] = valueToInject[key] + rawDefines[key] = config.DefineData{DefineFunc: func(args config.DefineArgs) js_ast.E { + return &js_ast.EIdentifier{Ref: args.SymbolForDefine(index)} + }} + } + for _, key := range pureFns { // The key must be a dot-separated identifier list for _, part := range strings.Split(key, ".") { @@ -396,7 +417,7 @@ func validateDefines(log logger.Log, defines map[string]string, pureFns []string // Processing defines is expensive. Process them once here so the same object // can be shared between all parsers we create using these arguments. processed := config.ProcessDefines(rawDefines) - return &processed + return &processed, injectedDefines } func validatePath(log logger.Log, fs fs.FS, relPath string) string { @@ -514,6 +535,7 @@ func rebuildImpl( realFS := fs.RealFS() jsFeatures, cssFeatures := validateFeatures(log, buildOpts.Target, buildOpts.Engines) outJS, outCSS := validateOutputExtensions(log, buildOpts.OutExtensions) + defines, injectedDefines := validateDefines(log, buildOpts.Define, buildOpts.Pure) options := config.Options{ UnsupportedJSFeatures: jsFeatures, UnsupportedCSSFeatures: cssFeatures, @@ -521,7 +543,8 @@ func rebuildImpl( Factory: validateJSX(log, buildOpts.JSXFactory, "factory"), Fragment: validateJSX(log, buildOpts.JSXFragment, "fragment"), }, - Defines: validateDefines(log, buildOpts.Define, buildOpts.Pure), + Defines: defines, + InjectedDefines: injectedDefines, Platform: validatePlatform(buildOpts.Platform), SourceMap: validateSourceMap(buildOpts.Sourcemap), ExcludeSourcesContent: buildOpts.SourcesContent == SourcesContentExclude, @@ -766,11 +789,13 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult // Convert and validate the transformOpts jsFeatures, cssFeatures := validateFeatures(log, transformOpts.Target, transformOpts.Engines) + defines, injectedDefines := validateDefines(log, transformOpts.Define, transformOpts.Pure) options := config.Options{ UnsupportedJSFeatures: jsFeatures, UnsupportedCSSFeatures: cssFeatures, JSX: jsx, - Defines: validateDefines(log, transformOpts.Define, transformOpts.Pure), + Defines: defines, + InjectedDefines: injectedDefines, SourceMap: validateSourceMap(transformOpts.Sourcemap), ExcludeSourcesContent: transformOpts.SourcesContent == SourcesContentExclude, OutputFormat: validateFormat(transformOpts.Format), diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index 95a2fe2aa68..997011da349 100644 --- a/scripts/js-api-tests.js +++ b/scripts/js-api-tests.js @@ -171,6 +171,26 @@ let buildTests = { assert.strictEqual(require(output).result, 123) }, + async defineObject({ esbuild, testDir }) { + const input = path.join(testDir, 'in.js'); + const output = path.join(testDir, 'out.js') + await writeFileAsync(input, 'export default {abc, xyz}') + await esbuild.build({ + entryPoints: [input], + outfile: output, + format: 'cjs', + bundle: true, + define: { + abc: '["a", "b", "c"]', + xyz: '{"x": 1, "y": 2, "z": 3}', + }, + }) + assert.deepStrictEqual(require(output).default, { + abc: ['a', 'b', 'c'], + xyz: { x: 1, y: 2, z: 3 }, + }) + }, + async inject({ esbuild, testDir }) { const input = path.join(testDir, 'in.js'); const inject = path.join(testDir, 'inject.js') @@ -1879,6 +1899,12 @@ let transformTests = { assert.strictEqual(code, `console.log("production");\n`) }, + async defineArray({ service }) { + const define = { 'process.env.NODE_ENV': '[1,2,3]', 'something.else': '[2,3,4]' } + const { code } = await service.transform(`console.log(process.env.NODE_ENV)`, { define }) + assert.strictEqual(code, `var define_process_env_NODE_ENV_default = [1, 2, 3];\nconsole.log(define_process_env_NODE_ENV_default);\n`) + }, + async defineWarning({ service }) { const define = { 'process.env.NODE_ENV': 'production' } const { code, warnings } = await service.transform(`console.log(process.env.NODE_ENV)`, { define })