Skip to content

Commit

Permalink
fix #3115: pass through unknown js directives
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed May 12, 2023
1 parent d686756 commit c19689a
Show file tree
Hide file tree
Showing 9 changed files with 247 additions and 53 deletions.
17 changes: 17 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -2,6 +2,23 @@

## Unreleased

* Avoid removing unrecognized directives from the directive prologue when minifying ([#3115](https://github.com/evanw/esbuild/issues/3115))

The [directive prologue](https://262.ecma-international.org/6.0/#sec-directive-prologues-and-the-use-strict-directive) in JavaScript is a sequence of top-level string expressions that come before your code. The only directives that JavaScript engines currently recognize are `use strict` and sometimes `use asm`. However, the people behind React have made up their own directive for their own custom dialect of JavaScript. Previously esbuild only preserved the `use strict` directive when minifying, although you could still write React JavaScript with esbuild using something like `--banner:js="'your directive here';"`. With this release, you can now put arbitrary directives in the entry point and esbuild will preserve them in its minified output:

```js
// Original code
'use wtf'; console.log(123)

// Old output (with --minify)
console.log(123);

// New output (with --minify)
"use wtf";console.log(123);
```

Note that this means esbuild will no longer remove certain stray top-level strings when minifying. This behavior is an intentional change because these stray top-level strings are actually part of the directive prologue, and could potentially have semantics assigned to them (as was the case with React).

* Improved minification of binary shift operators

With this release, esbuild's minifier will now evaluate the `<<` and `>>>` operators if the resulting code would be shorter:
Expand Down
102 changes: 102 additions & 0 deletions internal/bundler_tests/bundler_dce_test.go
View file
Expand Up @@ -3645,3 +3645,105 @@ func TestTreeShakingJSWithAssociatedCSSUnusedNestedImportSideEffectsFalseOnlyJS(
},
})
}

func TestPreserveDirectivesMinifyPassThrough(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
//! 1
'use 1'
//! 2
'use 2'
//! 3
'use 3'
entry()
//! 4
'use 4'
//! 5
'use 5'
//! 6
'use 6'
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModePassThrough,
AbsOutputFile: "/out.js",
MinifySyntax: true,
},
})
}

func TestPreserveDirectivesMinifyIIFE(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
//! 1
'use 1'
//! 2
'use 2'
//! 3
'use 3'
entry()
//! 4
'use 4'
//! 5
'use 5'
//! 6
'use 6'
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeConvertFormat,
OutputFormat: config.FormatIIFE,
AbsOutputFile: "/out.js",
MinifySyntax: true,
},
})
}

func TestPreserveDirectivesMinifyBundle(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
//! 1
'use 1'
//! 2
'use 2'
//! 3
'use 3'
entry()
//! 4
'use 4'
//! 5
'use 5'
//! 6
'use 6'
import "./nested.js"
`,
"/nested.js": `
//! A
'use A'
//! B
'use B'
//! C
'use C'
nested()
//! D
'use D'
//! E
'use E'
//! F
'use F'
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
OutputFormat: config.FormatIIFE,
AbsOutputFile: "/out.js",
MinifySyntax: true,
},
})
}
57 changes: 57 additions & 0 deletions internal/bundler_tests/snapshots/snapshots_dce.txt
View file
Expand Up @@ -197,6 +197,7 @@ var ns;

---------- /out/directive-before.js ----------
function nested() {
"directive";
x = [1, 1];
}

Expand Down Expand Up @@ -1505,6 +1506,62 @@ console.log("hello");
// Users/user/project/src/entry.js
console.log("unused import");

================================================================================
TestPreserveDirectivesMinifyBundle
---------- /out.js ----------
"use 1";
"use 2";
"use 3";
(() => {
// nested.js
//! A
//! B
//! C
nested();
//! D
//! E
//! F

// entry.js
//! 1
//! 2
//! 3
entry();
//! 4
//! 5
//! 6
})();

================================================================================
TestPreserveDirectivesMinifyIIFE
---------- /out.js ----------
"use 1";
"use 2";
"use 3";
(() => {
//! 1
//! 2
//! 3
entry();
//! 4
//! 5
//! 6
})();

================================================================================
TestPreserveDirectivesMinifyPassThrough
---------- /out.js ----------
"use 1";
"use 2";
"use 3";
//! 1
//! 2
//! 3
entry();
//! 4
//! 5
//! 6

================================================================================
TestPureCallsWithSpread
---------- /out.js ----------
Expand Down
2 changes: 1 addition & 1 deletion internal/bundler_tests/snapshots/snapshots_default.txt
View file
Expand Up @@ -5896,7 +5896,7 @@ TestUseStrictDirectiveBundleIssue1837
================================================================================
TestUseStrictDirectiveMinifyNoBundle
---------- /out.js ----------
"use strict";a,b;
"use strict";"use loose";a,b;

================================================================================
TestVarRelocatingBundle
Expand Down
6 changes: 3 additions & 3 deletions internal/js_ast/js_ast.go
View file
Expand Up @@ -1841,9 +1841,9 @@ type AST struct {
// This is internal-only data used for the implementation of Yarn PnP
ManifestForYarnPnP Expr

Hashbang string
Directive string
URLForCSS string
Hashbang string
Directives []string
URLForCSS string

// Note: If you're in the linker, do not use this map directly. This map is
// filled in by the parser and is considered immutable. For performance reasons,
Expand Down
76 changes: 45 additions & 31 deletions internal/js_parser/js_parser.go
View file
Expand Up @@ -8181,15 +8181,6 @@ func (p *parser) visitStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt {
return p.mangleStmts(visited, kind)
}

func isDirectiveSupported(s *js_ast.SDirective) bool {
// When minifying, strip all directives other than "use strict" since
// that should be the only one that is ever really used by engines in
// practice. We don't support "use asm" even though that's also
// technically used in practice because the rest of our minifier would
// likely cause asm.js code to fail validation anyway.
return helpers.UTF16EqualsString(s.Value, "use strict")
}

func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt {
// Remove inlined constants now that we know whether any of these statements
// contained a direct eval() or not. This can't be done earlier when we
Expand Down Expand Up @@ -8314,11 +8305,6 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt
// Strip empty statements
continue

case *js_ast.SDirective:
if !isDirectiveSupported(s) {
continue
}

case *js_ast.SLocal:
// Merge adjacent local statements
if len(result) > 0 {
Expand Down Expand Up @@ -15786,26 +15772,54 @@ func Parse(log logger.Log, source logger.Source, options Options) (result js_ast
p.prepareForVisitPass()

// Insert a "use strict" directive if "alwaysStrict" is active
directive := ""
var directives []string
if tsAlwaysStrict := p.options.tsAlwaysStrict; tsAlwaysStrict != nil && tsAlwaysStrict.Value {
directive = "use strict"
directives = append(directives, "use strict")
}

// Strip off a leading "use strict" directive when not bundling
for i, stmt := range stmts {
switch s := stmt.Data.(type) {
case *js_ast.SComment:
continue
case *js_ast.SDirective:
if !isDirectiveSupported(s) {
// Strip off all leading directives
{
totalCount := 0
keptCount := 0

for _, stmt := range stmts {
switch s := stmt.Data.(type) {
case *js_ast.SComment:
stmts[keptCount] = stmt
keptCount++
totalCount++
continue

case *js_ast.SDirective:
if p.isStrictMode() && s.LegacyOctalLoc.Start > 0 {
p.markStrictModeFeature(legacyOctalEscape, p.source.RangeOfLegacyOctalEscape(s.LegacyOctalLoc), "")
}
directive := helpers.UTF16ToString(s.Value)

// Remove duplicate directives
found := false
for _, existing := range directives {
if existing == directive {
found = true
break
}
}
if !found {
directives = append(directives, directive)
}

// Remove this directive from the statement list
totalCount++
continue
}
directive = helpers.UTF16ToString(s.Value)

// Remove this directive from the statement list
stmts = append(stmts[:i], stmts[i+1:]...)
// Stop when the directive prologue ends
break
}

if keptCount < totalCount {
stmts = append(stmts[:keptCount], stmts[totalCount:]...)
}
break
}

// Add an empty part for the namespace export that we can fill in later
Expand Down Expand Up @@ -15985,7 +15999,7 @@ func Parse(log logger.Log, source logger.Source, options Options) (result js_ast
// Pop the module scope to apply the "ContainsDirectEval" rules
p.popScope()

result = p.toAST(before, parts, after, hashbang, directive)
result = p.toAST(before, parts, after, hashbang, directives)
result.SourceMapComment = p.lexer.SourceMappingURL
return
}
Expand Down Expand Up @@ -16016,7 +16030,7 @@ func LazyExportAST(log logger.Log, source logger.Source, options Options, expr j
}
p.symbolUses = nil

ast := p.toAST(nil, []js_ast.Part{nsExportPart, part}, nil, "", "")
ast := p.toAST(nil, []js_ast.Part{nsExportPart, part}, nil, "", nil)
ast.HasLazyExport = true
return ast
}
Expand Down Expand Up @@ -16399,7 +16413,7 @@ func sortedKeysOfMapStringLocRef(in map[string]js_ast.LocRef) []string {
return keys
}

func (p *parser) toAST(before, parts, after []js_ast.Part, hashbang string, directive string) js_ast.AST {
func (p *parser) toAST(before, parts, after []js_ast.Part, hashbang string, directives []string) js_ast.AST {
// Insert an import statement for any runtime imports we generated
if len(p.runtimeImports) > 0 && !p.options.omitRuntimeForTests {
keys := sortedKeysOfMapStringLocRef(p.runtimeImports)
Expand Down Expand Up @@ -16603,7 +16617,7 @@ func (p *parser) toAST(before, parts, after []js_ast.Part, hashbang string, dire
ModuleRef: p.moduleRef,
WrapperRef: wrapperRef,
Hashbang: hashbang,
Directive: directive,
Directives: directives,
NamedImports: p.namedImports,
NamedExports: p.namedExports,
TSEnums: p.tsEnums,
Expand Down
8 changes: 4 additions & 4 deletions internal/js_parser/js_parser_test.go
View file
Expand Up @@ -4329,10 +4329,10 @@ func TestMangleUnused(t *testing.T) {
expectPrintedNormalAndMangle(t, "true", "true;\n", "")
expectPrintedNormalAndMangle(t, "123", "123;\n", "")
expectPrintedNormalAndMangle(t, "123n", "123n;\n", "")
expectPrintedNormalAndMangle(t, "'abc'", "\"abc\";\n", "") // Technically a directive, not a string expression
expectPrintedNormalAndMangle(t, "0; 'abc'", "0;\n\"abc\";\n", "") // Actually a string expression
expectPrintedNormalAndMangle(t, "'abc'; 'use strict'", "\"use strict\";\n\"abc\";\n", "\"use strict\";\n")
expectPrintedNormalAndMangle(t, "function f() { 'abc'; 'use strict' }", "function f() {\n \"abc\";\n \"use strict\";\n}\n", "function f() {\n \"use strict\";\n}\n")
expectPrintedNormalAndMangle(t, "'abc'", "\"abc\";\n", "\"abc\";\n") // Technically a directive, not a string expression
expectPrintedNormalAndMangle(t, "0; 'abc'", "0;\n\"abc\";\n", "") // Actually a string expression
expectPrintedNormalAndMangle(t, "'abc'; 'use strict'", "\"abc\";\n\"use strict\";\n", "\"abc\";\n\"use strict\";\n")
expectPrintedNormalAndMangle(t, "function f() { 'abc'; 'use strict' }", "function f() {\n \"abc\";\n \"use strict\";\n}\n", "function f() {\n \"abc\";\n \"use strict\";\n}\n")
expectPrintedNormalAndMangle(t, "this", "this;\n", "")
expectPrintedNormalAndMangle(t, "/regex/", "/regex/;\n", "")
expectPrintedNormalAndMangle(t, "(function() {})", "(function() {\n});\n", "")
Expand Down
4 changes: 2 additions & 2 deletions internal/js_printer/js_printer.go
View file
Expand Up @@ -4608,9 +4608,9 @@ func Print(tree js_ast.AST, symbols js_ast.SymbolMap, r renamer.Renamer, options
}

// Add the top-level directive if present
if tree.Directive != "" {
for _, directive := range tree.Directives {
p.printIndent()
p.printQuotedUTF8(tree.Directive, options.ASCIIOnly)
p.printQuotedUTF8(directive, options.ASCIIOnly)
p.print(";")
p.printNewline()
}
Expand Down

0 comments on commit c19689a

Please sign in to comment.