From e3090f11f390bac1d4cecc941b5603b647f48cfa Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 10 Sep 2022 11:38:11 -0400 Subject: [PATCH] fix(npm): remove export binding to match node (#15837) --- cli/node/mod.rs | 10 ++-------- cli/tests/integration/npm_tests.rs | 9 +++++++++ cli/tests/testdata/npm/cjs_this_in_exports/main.js | 11 +++++++++++ cli/tests/testdata/npm/cjs_this_in_exports/main.out | 5 +++++ .../@denotest/cjs-this-in-exports/1.0.0/index.js | 8 ++++++++ .../@denotest/cjs-this-in-exports/1.0.0/package.json | 5 +++++ .../testdata/npm/tarball_with_global_header/main.out | 2 +- cli/tests/testdata/npm/translate_cjs_to_esm/main.out | 2 +- ext/node/02_require.js | 11 ----------- 9 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 cli/tests/testdata/npm/cjs_this_in_exports/main.js create mode 100644 cli/tests/testdata/npm/cjs_this_in_exports/main.out create mode 100644 cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/index.js create mode 100644 cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/package.json diff --git a/cli/node/mod.rs b/cli/node/mod.rs index 9bc5b4a96e..91fde1db5e 100644 --- a/cli/node/mod.rs +++ b/cli/node/mod.rs @@ -814,20 +814,14 @@ pub fn translate_cjs_to_esm( for export in &all_exports { if export.as_str() == "default" { if root_exports.contains("__esModule") { - source.push(format!( - "export default Deno[Deno.internal].require.bindExport(mod[\"{}\"], mod);", - export, - )); + source.push(format!("export default mod[\"{}\"];", export)); had_default = true; } } else { add_export( &mut source, export, - &format!( - "Deno[Deno.internal].require.bindExport(mod[\"{}\"], mod)", - export - ), + &format!("mod[\"{}\"]", export), &mut temp_var_count, ); } diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index ab276a2da5..9073dfdb07 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -61,6 +61,14 @@ itest!(cjs_reexport_collision { http_server: true, }); +itest!(cjs_this_in_exports { + args: "run --allow-read --unstable --quiet npm/cjs_this_in_exports/main.js", + output: "npm/cjs_this_in_exports/main.out", + envs: env_vars(), + http_server: true, + exit_code: 1, +}); + itest!(translate_cjs_to_esm { args: "run --unstable -A --quiet npm/translate_cjs_to_esm/main.js", output: "npm/translate_cjs_to_esm/main.out", @@ -347,6 +355,7 @@ fn env_vars_no_sync_download() -> Vec<(String, String)> { "DENO_NPM_REGISTRY".to_string(), "http://localhost:4545/npm/registry/".to_string(), ), + ("NO_COLOR".to_string(), "1".to_string()), ] } diff --git a/cli/tests/testdata/npm/cjs_this_in_exports/main.js b/cli/tests/testdata/npm/cjs_this_in_exports/main.js new file mode 100644 index 0000000000..03aaabe059 --- /dev/null +++ b/cli/tests/testdata/npm/cjs_this_in_exports/main.js @@ -0,0 +1,11 @@ +import defaultImport, { getValue } from "npm:@denotest/cjs-this-in-exports"; +import * as namespaceImport from "npm:@denotest/cjs-this-in-exports"; + +console.log(defaultImport.getValue()); +// In Node this actually fails, but it seems to work in Deno +// so I guess there's no harm in that. +console.log(namespaceImport.getValue()); + +// This will throw because it's lost its context. +// (same thing occurs with Node's cjs -> esm translation) +getValue(); diff --git a/cli/tests/testdata/npm/cjs_this_in_exports/main.out b/cli/tests/testdata/npm/cjs_this_in_exports/main.out new file mode 100644 index 0000000000..653a62335b --- /dev/null +++ b/cli/tests/testdata/npm/cjs_this_in_exports/main.out @@ -0,0 +1,5 @@ +1 +1 +error: Uncaught TypeError: this.otherMethod is not a function + at getValue (file://[WILDCARD]/@denotest/cjs-this-in-exports/1.0.0/index.js:3:17) + at file://[WILDCARD]/testdata/npm/cjs_this_in_exports/main.js:11:1 diff --git a/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/index.js b/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/index.js new file mode 100644 index 0000000000..21a9d7d7e9 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/index.js @@ -0,0 +1,8 @@ +module.exports = { + getValue() { + return this.otherMethod(); + }, + otherMethod() { + return 1; + }, +}; diff --git a/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/package.json new file mode 100644 index 0000000000..729b8c34e6 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/cjs-this-in-exports/1.0.0/package.json @@ -0,0 +1,5 @@ +{ + "name": "@denotest/cjs-this-in-exports", + "version": "1.0.0", + "main": "./index.js" +} diff --git a/cli/tests/testdata/npm/tarball_with_global_header/main.out b/cli/tests/testdata/npm/tarball_with_global_header/main.out index c8016c3622..2f309499ee 100644 --- a/cli/tests/testdata/npm/tarball_with_global_header/main.out +++ b/cli/tests/testdata/npm/tarball_with_global_header/main.out @@ -1 +1 @@ -[Function: bound Client] +[Function: Client] diff --git a/cli/tests/testdata/npm/translate_cjs_to_esm/main.out b/cli/tests/testdata/npm/translate_cjs_to_esm/main.out index a1f3313312..c472472f39 100644 --- a/cli/tests/testdata/npm/translate_cjs_to_esm/main.out +++ b/cli/tests/testdata/npm/translate_cjs_to_esm/main.out @@ -1,2 +1,2 @@ [Function: access] -[Function: bound createApp] +[Function: createApp] diff --git a/ext/node/02_require.js b/ext/node/02_require.js index 582d42194c..f3ed266c3e 100644 --- a/ext/node/02_require.js +++ b/ext/node/02_require.js @@ -13,7 +13,6 @@ ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSplice, - FunctionPrototypeBind, ObjectGetOwnPropertyDescriptor, ObjectGetPrototypeOf, ObjectPrototypeHasOwnProperty, @@ -864,15 +863,6 @@ throw new Error("not implemented"); } - function bindExport(value, mod) { - // ensure exported functions are bound to their module object - if (typeof value === "function") { - return FunctionPrototypeBind(value, mod); - } else { - return value; - } - } - /** @param specifier {string} */ function packageSpecifierSubPath(specifier) { let parts = specifier.split("/"); @@ -892,7 +882,6 @@ toRealPath, cjsParseCache, readPackageScope, - bindExport, moduleExports: m, }, };