Skip to content

Commit

Permalink
avoid changing "this" for imported function calls
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Mar 3, 2021
1 parent 54ef111 commit ba6fa74
Show file tree
Hide file tree
Showing 9 changed files with 177 additions and 33 deletions.
25 changes: 25 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,31 @@

A file containing the contents `export {}` is still considered to be an ESM file even though it has no exports. However, if a file containing this edge case is converted to CommonJS internally during bundling (e.g. when it is the target of `require()`), esbuild failed to mark the `exports` symbol from the CommonJS wrapping closure as used even though it is actually needed. This resulted in an output file that crashed when run. The `exports` symbol is now considered used in this case, so the bug has been fixed.

* Avoid introducing `this` for imported function calls

It is possible to import a function exported by a CommonJS module into an ESM module like this:

```js
import {fn} from './cjs-file.js'
console.log(fn())
```

When you do this, esbuild currently transforms your code into something like this:

```js
var cjs_file = __toModule(require("./cjs-file.js"));
console.log(cjs_file.fn());
```

However, doing that changes the value of `this` observed by the export `fn`. The property access `cjs_file.fn` is in the syntactic "call target" position so the value of `this` becomes the value of `cjs_file`. With this release, esbuild will now use a different syntax in this case to avoid passing `cjs_file` as `this`:

```js
var cjs_file = __toModule(require("./cjs-file.js"));
console.log((0, cjs_file.fn)());
```

This change in esbuild mirrors a similar [recent TypeScript compiler change](https://github.com/microsoft/TypeScript/pull/35877), and also makes esbuild more consistent with Babel which already does this transformation.

## 0.8.54

* Fix ordering issue with private class methods ([#901](https://github.com/evanw/esbuild/issues/901))
Expand Down
31 changes: 31 additions & 0 deletions internal/bundler/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4308,3 +4308,34 @@ func TestVarRelocatingNoBundle(t *testing.T) {
},
})
}

func TestImportNamespaceThisValue(t *testing.T) {
default_suite.expectBundled(t, bundled{
files: map[string]string{
"/a.js": `
import def, * as ns from 'external'
console.log(ns[foo](), new ns[foo]())
`,
"/b.js": `
import def, * as ns from 'external'
console.log(ns.foo(), new ns.foo())
`,
"/c.js": `
import def, {foo} from 'external'
console.log(def(), foo())
console.log(new def(), new foo())
`,
},
entryPaths: []string{"/a.js", "/b.js", "/c.js"},
options: config.Options{
Mode: config.ModeBundle,
OutputFormat: config.FormatCommonJS,
AbsOutputDir: "/out",
ExternalModules: config.ExternalModules{
NodeModules: map[string]bool{
"external": true,
},
},
},
})
}
32 changes: 25 additions & 7 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ var require_bar = __commonJS((exports) => {
// entry.js
var import_foo = __toModule(require_foo());
var import_bar = __toModule(require_bar());
console.log(import_foo.foo(), import_bar.bar());
console.log((0, import_foo.foo)(), (0, import_bar.bar)());

================================================================================
TestEmptyExportClauseBundleAsCommonJSIssue910
Expand Down Expand Up @@ -984,7 +984,7 @@ var require_foo = __commonJS((exports) => {

// entry.js
var import_foo = __toModule(require_foo());
console.log(import_foo.default(import_foo.x, import_foo.y));
console.log((0, import_foo.default)(import_foo.x, import_foo.y));

================================================================================
TestImportMissingNeitherES6NorCommonJS
Expand Down Expand Up @@ -1034,6 +1034,24 @@ var require_foo = __commonJS(() => {
// import.js
console.log(Promise.resolve().then(() => __toModule(require_foo())));

================================================================================
TestImportNamespaceThisValue
---------- /out/a.js ----------
// a.js
var ns = __toModule(require("external"));
console.log(ns[foo](), new ns[foo]());

---------- /out/b.js ----------
// b.js
var ns = __toModule(require("external"));
console.log(ns.foo(), new ns.foo());

---------- /out/c.js ----------
// c.js
var import_external = __toModule(require("external"));
console.log((0, import_external.default)(), (0, import_external.foo)());
console.log(new import_external.default(), new import_external.foo());

================================================================================
TestImportReExportES6Issue149
---------- /out.js ----------
Expand Down Expand Up @@ -1178,7 +1196,7 @@ var require_custom_react = __commonJS((exports, module) => {

// entry.jsx
var import_custom_react = __toModule(require_custom_react());
console.log(/* @__PURE__ */ import_custom_react.elem("div", null), /* @__PURE__ */ import_custom_react.elem(import_custom_react.frag, null, "fragment"));
console.log(/* @__PURE__ */ (0, import_custom_react.elem)("div", null), /* @__PURE__ */ (0, import_custom_react.elem)(import_custom_react.frag, null, "fragment"));

================================================================================
TestJSXImportsES6
Expand Down Expand Up @@ -1706,7 +1724,7 @@ var require_foo = __commonJS((exports) => {
// entry.js
var import_foo = __toModule(require_foo());
(() => {
console.log(import_foo.fn());
console.log((0, import_foo.fn)());
})();

================================================================================
Expand Down Expand Up @@ -1758,7 +1776,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestOutbase
Expand Down Expand Up @@ -1838,7 +1856,7 @@ var require_bar = __commonJS((exports) => {
var import_bar = __toModule(require_bar());

// entry.js
import_bar.default();
(0, import_bar.default)();

================================================================================
TestReExportDefaultExternalCommonJS
Expand Down Expand Up @@ -2623,7 +2641,7 @@ var require_foo = __commonJS((exports) => {

// entry.js
var import_foo = __toModule(require_foo());
import_foo.foo();
(0, import_foo.foo)();

================================================================================
TestTypeofRequireBadPatterns
Expand Down
28 changes: 14 additions & 14 deletions internal/bundler/snapshots/snapshots_packagejson.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserIndexNoExt
Expand Down Expand Up @@ -64,7 +64,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapModuleToModule
Expand All @@ -86,7 +86,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapModuleToRelative
Expand All @@ -108,7 +108,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapNativeModuleDisabled
Expand All @@ -127,7 +127,7 @@ var require_demo_pkg = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_demo_pkg());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapRelativeDisabled
Expand All @@ -146,7 +146,7 @@ var require_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapRelativeToModule
Expand All @@ -166,7 +166,7 @@ var require_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserMapRelativeToRelative
Expand All @@ -186,7 +186,7 @@ var require_main_browser = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main_browser());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserNoExt
Expand Down Expand Up @@ -254,7 +254,7 @@ var require_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserOverModuleBrowser
Expand All @@ -268,7 +268,7 @@ var require_main_browser = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main_browser());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserString
Expand All @@ -282,7 +282,7 @@ var require_browser = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_browser());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserWithMainNode
Expand All @@ -296,7 +296,7 @@ var require_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonBrowserWithModuleBrowser
Expand Down Expand Up @@ -402,7 +402,7 @@ var require_custom_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_custom_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());

================================================================================
TestPackageJsonMainFieldsA
Expand Down Expand Up @@ -448,4 +448,4 @@ var require_main = __commonJS((exports, module) => {

// Users/user/project/src/entry.js
var import_demo_pkg = __toModule(require_main());
console.log(import_demo_pkg.default());
console.log((0, import_demo_pkg.default)());
10 changes: 5 additions & 5 deletions internal/bundler/snapshots/snapshots_tsconfig.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var require_util = __commonJS((exports, module) => {

// Users/user/project/src/app/entry.js
var import_util = __toModule(require_util());
console.log(import_util.default());
console.log((0, import_util.default)());

================================================================================
TestTsConfigJSX
Expand Down Expand Up @@ -190,7 +190,7 @@ var require_util = __commonJS((exports, module) => {

// Users/user/project/src/app/entry.js
var import_util = __toModule(require_util());
console.log(import_util.default());
console.log((0, import_util.default)());

================================================================================
TestTsconfigJsonBaseUrl
Expand All @@ -204,7 +204,7 @@ var require_util = __commonJS((exports, module) => {

// Users/user/project/src/app/entry.js
var import_util = __toModule(require_util());
console.log(import_util.default());
console.log((0, import_util.default)());

================================================================================
TestTsconfigJsonCommentAllowed
Expand All @@ -218,7 +218,7 @@ var require_util = __commonJS((exports, module) => {

// Users/user/project/src/app/entry.js
var import_util = __toModule(require_util());
console.log(import_util.default());
console.log((0, import_util.default)());

================================================================================
TestTsconfigJsonExtends
Expand Down Expand Up @@ -283,7 +283,7 @@ var require_util = __commonJS((exports, module) => {

// Users/user/project/src/app/entry.js
var import_util = __toModule(require_util());
console.log(import_util.default());
console.log((0, import_util.default)());

================================================================================
TestTsconfigPreserveUnusedImportClause
Expand Down
5 changes: 5 additions & 0 deletions internal/js_ast/js_ast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,11 @@ type EIdentifier struct {
// doing this instead of opt-out.
type EImportIdentifier struct {
Ref Ref

// If true, this was originally an identifier expression such as "foo". If
// false, this could potentially have been a member access expression such
// as "ns.foo" off of an imported namespace object.
WasOriginallyIdentifier bool
}

// This is similar to EIdentifier but it represents class-private fields and
Expand Down
Loading

0 comments on commit ba6fa74

Please sign in to comment.