From 65a4439ff7c469ffc654f497fe3dee6bb1fa2ddb Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sat, 23 Sep 2023 14:12:17 -0400 Subject: [PATCH] fix #3396: js decorator pretty-printing bugs --- CHANGELOG.md | 35 ++++++++++++++++ .../bundler_tests/snapshots/snapshots_dce.txt | 14 +++---- .../snapshots/snapshots_default.txt | 14 +++---- internal/js_printer/js_printer.go | 42 ++++++++++++++++--- 4 files changed, 85 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34d208d9d33..cbbe0d39f42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,41 @@ ## Unreleased +* Fix printing of JavaScript decorators in tricky cases ([#3396](https://github.com/evanw/esbuild/issues/3396)) + + This release fixes some bugs where esbuild's pretty-printing of JavaScript decorators could incorrectly produced code with a syntax error. The problem happened because esbuild sometimes substitutes identifiers for other expressions in the pretty-printer itself, but the decision about whether to wrap the expression or not didn't account for this. Here are some examples: + + ```js + // Original code + import { constant } from './constants.js' + import { imported } from 'external' + import { undef } from './empty.js' + class Foo { + @constant() + @imported() + @undef() + foo + } + + // Old output (with --bundle --format=cjs --packages=external --minify-syntax) + var import_external = require("external"); + var Foo = class { + @123() + @(0, import_external.imported)() + @(void 0)() + foo; + }; + + // New output (with --bundle --format=cjs --packages=external --minify-syntax) + var import_external = require("external"); + var Foo = class { + @(123()) + @((0, import_external.imported)()) + @((void 0)()) + foo; + }; + ``` + * Allow pre-release versions to be passed to `target` ([#3388](https://github.com/evanw/esbuild/issues/3388)) People want to be able to pass version numbers for unreleased versions of node (which have extra stuff after the version numbers) to esbuild's `target` setting and have esbuild do something reasonable with them. These version strings are of course not present in esbuild's internal feature compatibility table because an unreleased version has not been released yet (by definition). With this release, esbuild will now attempt to accept these version strings passed to `target` and do something reasonable with them. diff --git a/internal/bundler_tests/snapshots/snapshots_dce.txt b/internal/bundler_tests/snapshots/snapshots_dce.txt index 4c81cd9b827..cd64bd20afa 100644 --- a/internal/bundler_tests/snapshots/snapshots_dce.txt +++ b/internal/bundler_tests/snapshots/snapshots_dce.txt @@ -409,32 +409,32 @@ var fn = () => { }; // keep-these.js -var Class = @(fn) class { +var Class = @fn class { }; var Field = class { - @(fn) + @fn field; }; var Method = class { - @(fn) + @fn method() { } }; var Accessor = class { - @(fn) + @fn accessor accessor; }; var StaticField = class { - @(fn) + @fn static field; }; var StaticMethod = class { - @(fn) + @fn static method() { } }; var StaticAccessor = class { - @(fn) + @fn static accessor accessor; }; diff --git a/internal/bundler_tests/snapshots/snapshots_default.txt b/internal/bundler_tests/snapshots/snapshots_default.txt index fab5f422719..f30fa33c6fe 100644 --- a/internal/bundler_tests/snapshots/snapshots_default.txt +++ b/internal/bundler_tests/snapshots/snapshots_default.txt @@ -929,8 +929,8 @@ _ = class { #bar; classes = [ class { - @(import_somewhere.imported) - @(0, import_somewhere.imported)() + @import_somewhere.imported + @((0, import_somewhere.imported)()) imported; }, class { @@ -940,12 +940,12 @@ _ = class { }, class { @(123) - @123() + @(123()) constant; }, class { @(void 0) - @(void 0)() + @((void 0)()) undef; }, class { @@ -977,7 +977,7 @@ _ = class { #bar; classes = [ class { - @(imported) + @imported @imported() imported; }, @@ -988,12 +988,12 @@ _ = class { }, class { @(123) - @123() + @(123()) constant; }, class { @(void 0) - @(void 0)() + @((void 0)()) undef; }, class { diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go index 0b8b8f69950..18a0e7e65ef 100644 --- a/internal/js_printer/js_printer.go +++ b/internal/js_printer/js_printer.go @@ -964,15 +964,25 @@ const ( func (p *printer) printDecorators(decorators []js_ast.Decorator, how printDecorators) { for _, decorator := range decorators { wrap := false + wasCallTarget := false expr := decorator.Value outer: for { + isCallTarget := wasCallTarget + wasCallTarget = false + switch e := expr.Data.(type) { - case *js_ast.EIdentifier, *js_ast.ECall: + case *js_ast.EIdentifier: // "@foo" break outer + case *js_ast.ECall: + // "@foo()" + expr = e.Target + wasCallTarget = true + continue + case *js_ast.EDot: // "@foo.bar" if p.canPrintIdentifier(e.Name) { @@ -993,6 +1003,29 @@ func (p *printer) printDecorators(decorators []js_ast.Decorator, how printDecora // "@(foo[bar])" break + case *js_ast.EImportIdentifier: + ref := ast.FollowSymbols(p.symbols, e.Ref) + symbol := p.symbols.Get(ref) + + if symbol.ImportItemStatus == ast.ImportItemMissing { + // "@(void 0)" + break + } + + if symbol.NamespaceAlias != nil && isCallTarget && e.WasOriginallyIdentifier { + // "@((0, import_ns.fn)())" + break + } + + if value := p.options.ConstValues[ref]; value.Kind != js_ast.ConstValueNone { + // "@()" + break + } + + // "@foo" + // "@import_ns.fn" + break outer + default: // "@(foo + bar)" // "@(() => {})" @@ -3028,11 +3061,8 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla } else if symbol.NamespaceAlias != nil { wrap := p.callTarget == e && e.WasOriginallyIdentifier if wrap { - if p.options.MinifyWhitespace { - p.print("(0,") - } else { - p.print("(0, ") - } + p.print("(0,") + p.printSpace() } p.printSpaceBeforeIdentifier() p.addSourceMapping(expr.Loc)