Skip to content

Commit

Permalink
fix #758: subtle circular dependency issue
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Feb 8, 2021
1 parent cd08847 commit 81a1117
Show file tree
Hide file tree
Showing 11 changed files with 140 additions and 26 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@

Previously recursive symlinks (a symlink that points to another symlink) were an unhandled case in the path resolution algorithm. Now these cases should be supported up to a depth of 256 symlinks. This means esbuild's path resolution should now work with multi-level `yarn link` scenarios.

## 0.8.42
* Fix subtle circular dependency issue ([#758](https://github.com/evanw/esbuild/issues/758))

If esbuild is used to transform TypeScript to JavaScript without bundling (i.e. each file is transformed individually), the output format is CommonJS, and the original TypeScript code contains an import cycle where at least one of the links in the cycle is an `export * as` re-export statement, there could be certain situations where evaluating the transformed code results in an import being `undefined`. This is caused by the `__esModule` marker being added after the call to `require()` for the first transformed re-export statement. The fix was to move the marker to before the first call to `require()`. The `__esModule` marker is a convention from Babel that esbuild reuses which marks a module as being originally in the ECMAScript module format instead of the CommonJS module format.

* ## 0.8.42

* Fix crash with block-level function declaration and `--keep-names` ([#755](https://github.com/evanw/esbuild/issues/755))

Expand Down
41 changes: 35 additions & 6 deletions internal/bundler/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ type fileMeta struct {
// 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.
needsExportSymbolFromRuntime bool
needsExportSymbolFromRuntime bool
needsMarkAsModuleSymbolFromRuntime bool

// The index of the automatically-generated part used to represent the
// CommonJS wrapper. This part is empty and is only useful for tree shaking
Expand Down Expand Up @@ -1105,7 +1106,7 @@ func (c *linkerContext) scanImportsAndExports() {
//
// In that case the module *is* considered a CommonJS module because
// the namespace object must be created.
if record.ContainsImportStar && !otherRepr.ast.HasES6Syntax() && !otherRepr.ast.HasLazyExport {
if record.ContainsImportStar && !otherRepr.ast.HasES6ImportsOrExports() && !otherRepr.ast.HasLazyExport {
otherRepr.meta.cjsStyleExports = true
}

Expand Down Expand Up @@ -1318,11 +1319,17 @@ func (c *linkerContext) scanImportsAndExports() {
// Include the "__export" symbol from the runtime if it was used in the
// previous step. The previous step can't do this because it's running in
// parallel and can't safely mutate the "importsToBind" map of another file.
if repr.meta.needsExportSymbolFromRuntime {
if repr.meta.needsExportSymbolFromRuntime || repr.meta.needsMarkAsModuleSymbolFromRuntime {
runtimeRepr := c.files[runtime.SourceIndex].repr.(*reprJS)
exportRef := runtimeRepr.ast.ModuleScope.Members["__export"].Ref
exportPart := &repr.ast.Parts[repr.meta.nsExportPartIndex]
c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex)
if repr.meta.needsExportSymbolFromRuntime {
exportRef := runtimeRepr.ast.ModuleScope.Members["__export"].Ref
c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex)
}
if repr.meta.needsMarkAsModuleSymbolFromRuntime {
exportRef := runtimeRepr.ast.ModuleScope.Members["__markAsModule"].Ref
c.generateUseOfSymbolForInclude(exportPart, &repr.meta, 1, exportRef, runtime.SourceIndex)
}
}

for importRef, importToBind := range repr.meta.importsToBind {
Expand Down Expand Up @@ -1633,6 +1640,28 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) {
})
}

// If this file was originally ESM but is now in CommonJS, add a call to
// "__markAsModule" which sets the "__esModule" property to true. This must
// be done before any to "require()" or circular imports of multiple modules
// that have been each converted from ESM to CommonJS may not work correctly.
if repr.ast.HasES6Exports && (repr.meta.cjsStyleExports || (file.isEntryPoint && c.options.OutputFormat == config.FormatCommonJS)) {
runtimeRepr := c.files[runtime.SourceIndex].repr.(*reprJS)
markAsModuleRef := runtimeRepr.ast.ModuleScope.Members["__markAsModule"].Ref
nsExportStmts = append(nsExportStmts, js_ast.Stmt{Data: &js_ast.SExpr{Value: js_ast.Expr{Data: &js_ast.ECall{
Target: js_ast.Expr{Data: &js_ast.EIdentifier{Ref: markAsModuleRef}},
Args: []js_ast.Expr{{Data: &js_ast.EIdentifier{Ref: repr.ast.ExportsRef}}},
}}}})

// Make sure this file depends on the "__markAsModule" symbol
for _, partIndex := range runtimeRepr.ast.TopLevelSymbolToParts[markAsModuleRef] {
dep := partRef{sourceIndex: runtime.SourceIndex, partIndex: partIndex}
nsExportNonLocalDependencies = append(nsExportNonLocalDependencies, dep)
}

// Pull in the "__markAsModule" symbol later
repr.meta.needsMarkAsModuleSymbolFromRuntime = true
}

// "__export(exports, { foo: () => foo })"
exportRef := js_ast.InvalidRef
if len(properties) > 0 {
Expand Down Expand Up @@ -2201,7 +2230,7 @@ func (c *linkerContext) advanceImportTracker(tracker importTracker) (importTrack

// Is this a named import of a file without any exports?
otherRepr := c.files[otherSourceIndex].repr.(*reprJS)
if namedImport.Alias != "*" && !otherRepr.ast.UsesCommonJSExports() && !otherRepr.ast.HasES6Syntax() && !otherRepr.ast.HasLazyExport {
if namedImport.Alias != "*" && !otherRepr.ast.UsesCommonJSExports() && !otherRepr.ast.HasES6ImportsOrExports() && !otherRepr.ast.HasLazyExport {
// Just warn about it and replace the import with "undefined"
return importTracker{sourceIndex: otherSourceIndex, importRef: js_ast.InvalidRef}, importCommonJSWithoutExports, nil
}
Expand Down
3 changes: 3 additions & 0 deletions internal/bundler/snapshots/snapshots_dce.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ TestPackageJsonSideEffectsArrayKeepMainImplicitMain
---------- /out.js ----------
// Users/user/project/node_modules/demo-pkg/index-main.js
var require_index_main = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
foo: () => foo2
});
Expand Down Expand Up @@ -185,6 +186,7 @@ TestPackageJsonSideEffectsArrayKeepModuleImplicitMain
---------- /out.js ----------
// Users/user/project/node_modules/demo-pkg/index-main.js
var require_index_main = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
foo: () => foo
});
Expand Down Expand Up @@ -246,6 +248,7 @@ TestPackageJsonSideEffectsFalseKeepBareImportAndRequireES6
---------- /out.js ----------
// Users/user/project/node_modules/demo-pkg/index.js
var require_demo_pkg = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
foo: () => foo
});
Expand Down
40 changes: 31 additions & 9 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ TestCommonJSFromES6
---------- /out.js ----------
// foo.js
var require_foo = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
foo: () => foo2
});
Expand All @@ -246,6 +247,7 @@ var require_foo = __commonJS((exports) => {

// bar.js
var require_bar = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
bar: () => bar2
});
Expand Down Expand Up @@ -473,6 +475,7 @@ TestExportFSNodeInCommonJSModule
import * as fs from "fs";
import {readFileSync} from "fs";
var require_entry = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
fs: () => fs,
readFileSync: () => readFileSync
Expand All @@ -486,6 +489,7 @@ TestExportFormsCommonJS
---------- /out.js ----------
// commonjs.js
var require_commonjs = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
C: () => Class,
Class: () => Class,
Expand All @@ -509,6 +513,7 @@ var require_commonjs = __commonJS((exports) => {

// c.js
var require_c = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => c_default2
});
Expand All @@ -519,6 +524,7 @@ var require_c = __commonJS((exports) => {

// d.js
var require_d = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => d_default
});
Expand All @@ -530,6 +536,7 @@ var require_d = __commonJS((exports) => {

// e.js
var require_e = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => e_default
});
Expand All @@ -539,6 +546,7 @@ var require_e = __commonJS((exports) => {

// f.js
var require_f = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => foo
});
Expand All @@ -549,6 +557,7 @@ var require_f = __commonJS((exports) => {

// g.js
var require_g = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => g_default
});
Expand All @@ -558,6 +567,7 @@ var require_g = __commonJS((exports) => {

// h.js
var require_h = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => foo
});
Expand Down Expand Up @@ -698,6 +708,7 @@ export default class o {
TestExportWildcardFSNodeCommonJS
---------- /out.js ----------
// entry.js
__markAsModule(exports);
__exportStar(exports, __toModule(require("fs")));

================================================================================
Expand Down Expand Up @@ -732,6 +743,7 @@ TestExternalES6ConvertedToCommonJS
// a.js
import * as ns from "x";
var require_a = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
ns: () => ns
});
Expand All @@ -740,6 +752,7 @@ var require_a = __commonJS((exports) => {
// b.js
import * as ns2 from "x";
var require_b = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
ns: () => ns2
});
Expand All @@ -748,6 +761,7 @@ var require_b = __commonJS((exports) => {
// c.js
import * as ns3 from "x";
var require_c = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
ns: () => ns3
});
Expand All @@ -756,6 +770,7 @@ var require_c = __commonJS((exports) => {
// d.js
import {ns as ns4} from "x";
var require_d = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
ns: () => ns4
});
Expand All @@ -764,6 +779,7 @@ var require_d = __commonJS((exports) => {
// e.js
import * as x_star from "x";
var require_e = __commonJS((exports) => {
__markAsModule(exports);
__exportStar(exports, x_star);
});

Expand Down Expand Up @@ -1418,21 +1434,21 @@ Promise.resolve().then(function(){return __toModule(require("foo"))});Promise.re
TestMinifiedExportsAndModuleFormatCommonJS
---------- /out.js ----------
// foo/test.js
var t = {};
f(t, {
foo: () => l
var o = {};
s(o, {
foo: () => p
});
var l = 123;
var p = 123;

// bar/test.js
var r = {};
f(r, {
bar: () => m
var t = {};
s(t, {
bar: () => l
});
var m = 123;
var l = 123;

// entry.js
console.log(exports, module.exports, t, r);
console.log(exports, module.exports, o, t);

================================================================================
TestMinifyArguments
Expand Down Expand Up @@ -1698,6 +1714,7 @@ TestReExportDefaultCommonJS
---------- /out.js ----------
// bar.js
var require_bar = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
default: () => foo
});
Expand All @@ -1716,6 +1733,7 @@ import_bar.default();
TestReExportDefaultExternalCommonJS
---------- /out.js ----------
// entry.js
__markAsModule(exports);
__export(exports, {
bar: () => import_bar.default,
foo: () => import_foo.default
Expand Down Expand Up @@ -1760,6 +1778,7 @@ export {default as bar} from "./bar";
================================================================================
TestReExportDefaultNoBundleCommonJS
---------- /out.js ----------
__markAsModule(exports);
__export(exports, {
bar: () => import_bar.default,
foo: () => import_foo.default
Expand Down Expand Up @@ -2173,6 +2192,7 @@ var require_cjs = __commonJS((exports) => {

// dummy.js
var require_dummy = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
dummy: () => dummy
});
Expand All @@ -2199,6 +2219,7 @@ var require_es6_expr_import_dynamic = __commonJS((exports) => {

// es6-export-star.js
var require_es6_export_star = __commonJS((exports) => {
__markAsModule(exports);
__exportStar(exports, __toModule(require_dummy()));
console.log(void 0);
});
Expand Down Expand Up @@ -2401,6 +2422,7 @@ TestTopLevelReturn
---------- /out.js ----------
// foo.js
var require_foo = __commonJS((exports) => {
__markAsModule(exports);
__export(exports, {
foo: () => foo2
});
Expand Down
Loading

0 comments on commit 81a1117

Please sign in to comment.