From 445966fcb1afd86713e4117372723939d25e6843 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 8 Sep 2019 11:20:43 -0400 Subject: [PATCH 1/5] module: error for CJS .js load within type: module --- lib/internal/modules/cjs/loader.js | 73 +++++++++++---------- test/es-module/test-esm-type-flag-errors.js | 7 ++ 2 files changed, 47 insertions(+), 33 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index db93c6b787baca..2c16754585af43 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -212,16 +212,17 @@ Module._debug = deprecate(debug, 'Module._debug is deprecated.', 'DEP0077'); // -> a. // -> a/index. -// Check if the directory is a package.json dir. -const packageMainCache = Object.create(null); -// Explicit exports from package.json files -const packageExportsCache = new SafeMap(); +const packageJsonCache = new SafeMap(); -function readPackageRaw(requestPath) { +function readPackage(requestPath) { const jsonPath = path.resolve(requestPath, 'package.json'); - const json = internalModuleReadJSON(path.toNamespacedPath(jsonPath)); + const existing = packageJsonCache.get(jsonPath); + if (existing !== undefined) return existing; + + const json = internalModuleReadJSON(path.toNamespacedPath(jsonPath)); if (json === undefined) { + packageJsonCache.set(jsonPath, false); return false; } @@ -232,11 +233,13 @@ function readPackageRaw(requestPath) { try { const parsed = JSON.parse(json); - packageMainCache[requestPath] = parsed.main; - if (experimentalExports) { - packageExportsCache.set(requestPath, parsed.exports); - } - return parsed; + const filtered = { + main: parsed.main, + exports: parsed.exports, + type: parsed.type + }; + packageJsonCache.set(jsonPath, filtered); + return filtered; } catch (e) { e.path = jsonPath; e.message = 'Error parsing ' + jsonPath + ': ' + e.message; @@ -244,33 +247,33 @@ function readPackageRaw(requestPath) { } } -function readPackage(requestPath) { - const entry = packageMainCache[requestPath]; - if (entry) - return entry; - - const pkg = readPackageRaw(requestPath); - if (pkg === false) return false; - - return pkg.main; -} - -function readExports(requestPath) { - if (packageExportsCache.has(requestPath)) { - return packageExportsCache.get(requestPath); +function readPackageScope(checkPath) { + const rootSeparatorIndex = checkPath.indexOf(path.sep); + let separatorIndex; + while ( + (separatorIndex = checkPath.lastIndexOf(path.sep)) > rootSeparatorIndex + ) { + checkPath = checkPath.slice(0, separatorIndex); + if (checkPath.endsWith(path.sep + 'node_modules')) + return false; + const pjson = readPackage(checkPath); + if (pjson) return pjson; } + return false; +} - const pkg = readPackageRaw(requestPath); - if (!pkg) { - packageExportsCache.set(requestPath, null); - return null; - } +function readPackageMain(requestPath) { + const pkg = readPackage(requestPath); + return pkg ? pkg.main : undefined; +} - return pkg.exports; +function readPackageExports(requestPath) { + const pkg = readPackage(requestPath); + return pkg ? pkg.exports : undefined; } function tryPackage(requestPath, exts, isMain, originalPath) { - const pkg = readPackage(requestPath); + const pkg = readPackageMain(requestPath); if (!pkg) { return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain); @@ -371,7 +374,7 @@ function resolveExports(nmPath, request, absoluteRequest) { } const basePath = path.resolve(nmPath, name); - const pkgExports = readExports(basePath); + const pkgExports = readPackageExports(basePath); const mappingKey = `.${expansion}`; if (typeof pkgExports === 'object' && pkgExports !== null) { @@ -943,6 +946,10 @@ Module.prototype._compile = function(content, filename) { // Native extension for .js Module._extensions['.js'] = function(module, filename) { + const pkg = readPackageScope(filename); + if (pkg && pkg.type === 'module') { + throw new ERR_REQUIRE_ESM(filename); + } const content = fs.readFileSync(filename, 'utf8'); module._compile(content, filename); }; diff --git a/test/es-module/test-esm-type-flag-errors.js b/test/es-module/test-esm-type-flag-errors.js index 169118e01800ef..a54a018ad9b774 100644 --- a/test/es-module/test-esm-type-flag-errors.js +++ b/test/es-module/test-esm-type-flag-errors.js @@ -23,6 +23,13 @@ expect('', packageWithoutTypeMain, 'package-without-type'); expect('--input-type=module', packageTypeModuleMain, 'ERR_INPUT_TYPE_NOT_ALLOWED', true); +try { + require('../fixtures/es-modules/package-type-module/index.js'); + assert.fail('Expected CJS to fail loading from type: module package.'); +} catch (e) { + assert(e.toString().match(/Error \[ERR_REQUIRE_ESM\]: Must use import to load ES Module:/)); +} + function expect(opt = '', inputFile, want, wantsError = false) { // TODO: Remove when --experimental-modules is unflagged opt = `--experimental-modules ${opt}`; From be7c4db6adecba7a4f5ded681bbdc3841436b24d Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 8 Sep 2019 11:50:35 -0400 Subject: [PATCH 2/5] fixup policy tests --- test/parallel/test-policy-integrity.js | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/parallel/test-policy-integrity.js b/test/parallel/test-policy-integrity.js index 722b45f633174d..2cc1f1280c8059 100644 --- a/test/parallel/test-policy-integrity.js +++ b/test/parallel/test-policy-integrity.js @@ -212,6 +212,10 @@ test({ shouldFail: false, entry: parentFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [parentURL]: { body: parentBody, match: true, @@ -227,6 +231,10 @@ test({ preload: [depFilepath], entry: parentFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [parentURL]: { body: parentBody, match: true, @@ -279,6 +287,10 @@ test({ shouldFail: false, entry: depFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [depURL]: { body: depBody, match: true, @@ -289,6 +301,10 @@ test({ shouldFail: false, entry: depFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [policyToDepRelativeURLString]: { body: depBody, match: true, @@ -309,6 +325,10 @@ test({ shouldFail: false, entry: depFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [policyToDepRelativeURLString]: { body: depBody, match: true, @@ -351,6 +371,10 @@ test({ shouldFail: false, entry: workerSpawningFilepath, resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [workerSpawningURL]: { body: workerSpawningBody, match: true, @@ -370,6 +394,10 @@ test({ entry: workerSpawningFilepath, preload: [parentFilepath], resources: { + [packageURL]: { + body: packageBody, + match: true, + }, [workerSpawningURL]: { body: workerSpawningBody, match: true, From 38ff1ea091dc8684fdad972fc3e2789a898387dd Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 17 Sep 2019 16:04:26 -0400 Subject: [PATCH 3/5] add "type" to package.json filter --- src/node_file.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index ae2aa0677f6e89..195757f43f3dc2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -875,7 +875,8 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { const size_t size = offset - start; if (size == 0 || ( size == SearchString(&chars[start], size, "\"main\"") && - size == SearchString(&chars[start], size, "\"exports\""))) { + size == SearchString(&chars[start], size, "\"exports\"") && + size == SearchString(&chars[start], size, "\"type\""))) { return; } else { Local chars_string = From d85bfffb0fd20e9312be0a9283fa8ba2932660d6 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 18 Sep 2019 10:45:24 -0400 Subject: [PATCH 4/5] explicit .js extension check --- lib/internal/modules/cjs/loader.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 2c16754585af43..c0425429dd2f79 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -946,9 +946,11 @@ Module.prototype._compile = function(content, filename) { // Native extension for .js Module._extensions['.js'] = function(module, filename) { - const pkg = readPackageScope(filename); - if (pkg && pkg.type === 'module') { - throw new ERR_REQUIRE_ESM(filename); + if (filename.endsWith('.js')) { + const pkg = readPackageScope(filename); + if (pkg && pkg.type === 'module') { + throw new ERR_REQUIRE_ESM(filename); + } } const content = fs.readFileSync(filename, 'utf8'); module._compile(content, filename); From a6bfda0e145889cd7fee8e4c49b6040503e44764 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 18 Sep 2019 22:27:21 -0400 Subject: [PATCH 5/5] fixup integrity policy test --- test/parallel/test-policy-parse-integrity.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/parallel/test-policy-parse-integrity.js b/test/parallel/test-policy-parse-integrity.js index 7779f767fb97b7..6fa95416390693 100644 --- a/test/parallel/test-policy-parse-integrity.js +++ b/test/parallel/test-policy-parse-integrity.js @@ -36,8 +36,16 @@ if (!tmpdirURL.pathname.endsWith('/')) { tmpdirURL.pathname += '/'; } +const packageFilepath = path.join(tmpdir.path, 'package.json'); +const packageURL = pathToFileURL(packageFilepath); +const packageBody = '{"main": "dep.js"}'; + function test({ shouldFail, integrity }) { const resources = { + [packageURL]: { + body: packageBody, + integrity: `sha256-${hash('sha256', packageBody)}` + }, [depURL]: { body: depBody, integrity