Skip to content

Commit

Permalink
fix #542: cyclic imports with cjs-style output
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Nov 25, 2020
1 parent 1e182e7 commit 2446da5
Show file tree
Hide file tree
Showing 6 changed files with 224 additions and 104 deletions.
61 changes: 61 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,67 @@

With TypeScript 4.1, you can now omit `baseUrl` when using `paths`. When you do this, it as if you had written `"baseUrl": "."` instead for the purpose of the `paths` feature, but the `baseUrl` value is not actually set and does not affect path resolution. These `tsconfig.json` files are now supported by esbuild.

* Fix evaluation order issue with import cycles and CommonJS-style output formats ([#542](https://github.com/evanw/esbuild/issues/542))

Previously entry points involved in an import cycle could cause evaluation order issues if the output format was `iife` or `cjs` instead of `esm`. This happened because this edge case was handled by treating the entry point file as a CommonJS file, which extracted the code into a CommonJS wrapper. Here's an example:

Input files:

```js
// index.js
import { test } from './lib'
export function fn() { return 42 }
if (test() !== 42) throw 'failure'
```

```js
// lib.js
import { fn } from './index'
export let test = fn
```

Previous output (problematic):

```js
// index.js
var require_esbuild = __commonJS((exports) => {
__export(exports, {
fn: () => fn2
});
function fn2() {
return 42;
}
if (test() !== 42)
throw "failure";
});
// lib.js
var index = __toModule(require_esbuild());
var test = index.fn;
module.exports = require_esbuild();
```

This approach changed the evaluation order because the CommonJS wrapper conflates both binding and evaluation. Binding and evaluation need to be separated to correctly handle this edge case. This edge case is now handled by inlining what would have been the contents of the CommonJS wrapper into the entry point location itself.

Current output (fixed):

```js
// index.js
__export(exports, {
fn: () => fn
});
// lib.js
var test = fn;
// index.js
function fn() {
return 42;
}
if (test() !== 42)
throw "failure";
```

## 0.8.14

* Fix a concurrency bug caused by an error message change ([#556](https://github.com/evanw/esbuild/issues/556))
Expand Down
18 changes: 18 additions & 0 deletions internal/bundler/bundler_importstar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,24 @@ func TestExportSelfIIFE(t *testing.T) {
})
}

func TestExportSelfIIFEWithName(t *testing.T) {
importstar_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
export const foo = 123
export * from './entry'
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
OutputFormat: config.FormatIIFE,
AbsOutputFile: "/out.js",
ModuleName: []string{"someName"},
},
})
}

func TestExportSelfES6(t *testing.T) {
importstar_suite.expectBundled(t, bundled{
files: map[string]string{
Expand Down
105 changes: 63 additions & 42 deletions internal/bundler/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,13 @@ type fileMeta struct {
// this module must be added as getters to the CommonJS "exports" object.
cjsStyleExports bool

// If true, the "__export(exports, { ... })" call will be force-included even
// if there are no parts that reference "exports". Otherwise this call will
// be removed due to the tree shaking pass. This is used when for entry point
// files when code related to the current output format needs to reference
// the "exports" variable.
forceIncludeExportsForEntryPoint bool

// This is set when we need to pull in the "__export" symbol in to the part
// at "nsExportPartIndex". This can't be done in "createExportsForFile"
// because of concurrent map hazards. Instead, it must be done later.
Expand Down Expand Up @@ -416,12 +423,14 @@ func newLinkerContext(
file := &c.files[sourceIndex]
file.isEntryPoint = true

// Entry points must be CommonJS-style if the output format doesn't support
// ES6 export syntax
if !options.OutputFormat.KeepES6ImportExportSyntax() {
if repr, ok := file.repr.(*reprJS); ok && repr.ast.HasES6Exports {
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, ok := file.repr.(*reprJS); ok && repr.ast.HasES6Exports && (options.OutputFormat == config.FormatCommonJS ||
(options.OutputFormat == config.FormatIIFE && len(options.ModuleName) > 0)) {
repr.ast.UsesExportsRef = true
repr.meta.forceIncludeExportsForEntryPoint = true
}
}

Expand Down Expand Up @@ -1239,7 +1248,8 @@ func (c *linkerContext) scanImportsAndExports() {
// Step 6: Bind imports to exports. This adds non-local dependencies on the
// parts that declare the export to all parts that use the import.
for _, sourceIndex := range c.reachableFiles {
repr, ok := c.files[sourceIndex].repr.(*reprJS)
file := &c.files[sourceIndex]
repr, ok := file.repr.(*reprJS)
if !ok {
continue
}
Expand All @@ -1249,8 +1259,9 @@ func (c *linkerContext) scanImportsAndExports() {
// actual CommonJS files from being renamed. This is purely about
// aesthetics and is not about correctness. This is done here because by
// this point, we know the CommonJS status will not change further.
if !repr.meta.cjsWrap && !repr.meta.cjsStyleExports {
name := c.files[sourceIndex].source.IdentifierName
if !repr.meta.cjsWrap && !repr.meta.cjsStyleExports && (!file.isEntryPoint ||
c.options.OutputFormat != config.FormatCommonJS) {
name := file.source.IdentifierName
c.symbols.Get(repr.ast.ExportsRef).OriginalName = name + "_exports"
c.symbols.Get(repr.ast.ModuleRef).OriginalName = name + "_module"
}
Expand Down Expand Up @@ -1562,7 +1573,7 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) {
// Prefix this part with "var exports = {}" if this isn't a CommonJS module
declaredSymbols := []js_ast.DeclaredSymbol{}
var nsExportStmts []js_ast.Stmt
if !repr.meta.cjsStyleExports {
if !repr.meta.cjsStyleExports && (!file.isEntryPoint || c.options.OutputFormat != config.FormatCommonJS) {
nsExportStmts = append(nsExportStmts, js_ast.Stmt{Data: &js_ast.SLocal{Decls: []js_ast.Decl{{
Binding: js_ast.Binding{Data: &js_ast.BIdentifier{Ref: repr.ast.ExportsRef}},
Value: &js_ast.Expr{Data: &js_ast.EObject{}},
Expand Down Expand Up @@ -1635,44 +1646,49 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) {
// If we're an entry point, call the require function at the end of the
// bundle right before bundle evaluation ends
var cjsWrapStmt js_ast.Stmt
if file.isEntryPoint && repr.meta.cjsWrap {
switch c.options.OutputFormat {
case config.FormatPreserve:
// "require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SExpr{Value: js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}

case config.FormatIIFE:
if len(c.options.ModuleName) > 0 {
// "return require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SReturn{Value: &js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}
} else {
if file.isEntryPoint {
if repr.meta.cjsWrap {
switch c.options.OutputFormat {
case config.FormatPreserve:
// "require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SExpr{Value: js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}
}

case config.FormatCommonJS:
// "module.exports = require_foo();"
cjsWrapStmt = js_ast.AssignStmt(
js_ast.Expr{Data: &js_ast.EDot{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: c.unboundModuleRef}},
Name: "exports",
}},
js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}},
)
case config.FormatIIFE:
if len(c.options.ModuleName) > 0 {
// "return require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SReturn{Value: &js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}
} else {
// "require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SExpr{Value: js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}
}

case config.FormatESModule:
// "export default require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SExportDefault{Value: js_ast.ExprOrStmt{Expr: &js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}}
case config.FormatCommonJS:
// "module.exports = require_foo();"
cjsWrapStmt = js_ast.AssignStmt(
js_ast.Expr{Data: &js_ast.EDot{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: c.unboundModuleRef}},
Name: "exports",
}},
js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}},
)

case config.FormatESModule:
// "export default require_foo();"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SExportDefault{Value: js_ast.ExprOrStmt{Expr: &js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.WrapperRef}},
}}}}}
}
} else if repr.meta.forceIncludeExportsForEntryPoint && c.options.OutputFormat == config.FormatIIFE && len(c.options.ModuleName) > 0 {
// "return exports;"
cjsWrapStmt = js_ast.Stmt{Data: &js_ast.SReturn{Value: &js_ast.Expr{Data: &js_ast.EIdentifier{Ref: repr.ast.ExportsRef}}}}
}
}

Expand Down Expand Up @@ -2217,6 +2233,11 @@ func (c *linkerContext) includeFile(sourceIndex uint32, entryPointBit uint, dist
c.includePart(targetSourceIndex, partIndex, entryPointBit, distanceFromEntryPoint)
}
}

// Ensure "exports" is included if the current output format needs it
if repr.meta.forceIncludeExportsForEntryPoint {
c.includePart(sourceIndex, repr.meta.nsExportPartIndex, entryPointBit, distanceFromEntryPoint)
}
}

case *reprCSS:
Expand Down
43 changes: 22 additions & 21 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -587,26 +587,17 @@ TestExportFormsIIFE
---------- /out.js ----------
var moduleName = (() => {
// /entry.js
var require_entry = __commonJS((exports) => {
__export(exports, {
C: () => Class,
Class: () => Class,
Fn: () => Fn,
abc: () => abc,
b: () => b_exports,
c: () => c,
default: () => entry_default,
l: () => l,
v: () => v
});
var entry_default = 123;
var v = 234;
var l = 234;
var c = 234;
function Fn() {
}
var Class = class {
};
var entry_exports = {};
__export(entry_exports, {
C: () => Class,
Class: () => Class,
Fn: () => Fn,
abc: () => abc,
b: () => b_exports,
c: () => c,
default: () => entry_default,
l: () => l,
v: () => v
});

// /a.js
Expand All @@ -618,7 +609,17 @@ var moduleName = (() => {
xyz: () => xyz
});
var xyz = null;
return require_entry();

// /entry.js
var entry_default = 123;
var v = 234;
var l = 234;
var c = 234;
function Fn() {
}
var Class = class {
};
return entry_exports;
})();

================================================================================
Expand Down
Loading

0 comments on commit 2446da5

Please sign in to comment.