From f0850a310a154f13486b83c346c895828caec9e2 Mon Sep 17 00:00:00 2001 From: Rongjian Zhang Date: Sun, 24 Nov 2019 12:40:40 +0800 Subject: [PATCH] module: add warnings for experimental flags PR-URL: https://github.com/nodejs/node/pull/30617 Fixes: https://github.com/nodejs/node/issues/30600 Reviewed-By: Guy Bedford --- lib/internal/modules/cjs/loader.js | 15 +++++--- lib/internal/modules/esm/translators.js | 4 ++- src/module_wrap.cc | 3 ++ src/node_process.h | 2 ++ src/node_process_events.cc | 16 +++++++++ test/common/fixtures.mjs | 16 +++++++++ test/es-module/test-esm-exports.mjs | 36 +++++++++++++++++++ test/es-module/test-esm-json.mjs | 25 ++++++++++++- test/es-module/test-esm-wasm.mjs | 25 ++++++++++++- .../es-modules/conditional-exports.js | 1 + test/fixtures/es-modules/json-modules.mjs | 1 + test/fixtures/es-modules/wasm-modules.mjs | 2 ++ .../node_modules/pkgexports/resolve-self.js | 1 + 13 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 test/common/fixtures.mjs create mode 100644 test/fixtures/es-modules/conditional-exports.js create mode 100644 test/fixtures/es-modules/json-modules.mjs create mode 100644 test/fixtures/es-modules/wasm-modules.mjs create mode 100644 test/fixtures/node_modules/pkgexports/resolve-self.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 225aa1787b0c3f..be1a8559881275 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -36,7 +36,7 @@ const { rekeySourceMap } = require('internal/source_map/source_map_cache'); const { pathToFileURL, fileURLToPath, URL } = require('internal/url'); -const { deprecate } = require('internal/util'); +const { deprecate, emitExperimentalWarning } = require('internal/util'); const vm = require('vm'); const assert = require('internal/assert'); const fs = require('fs'); @@ -576,8 +576,10 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (experimentalConditionalExports && ObjectPrototype.hasOwnProperty(target, 'require')) { try { - return resolveExportsTarget(pkgPath, target.require, subpath, - basePath, mappingKey); + const result = resolveExportsTarget(pkgPath, target.require, subpath, + basePath, mappingKey); + emitExperimentalWarning('Conditional exports'); + return result; } catch (e) { if (e.code !== 'MODULE_NOT_FOUND') throw e; } @@ -585,8 +587,10 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (experimentalConditionalExports && ObjectPrototype.hasOwnProperty(target, 'node')) { try { - return resolveExportsTarget(pkgPath, target.node, subpath, - basePath, mappingKey); + const result = resolveExportsTarget(pkgPath, target.node, subpath, + basePath, mappingKey); + emitExperimentalWarning('Conditional exports'); + return result; } catch (e) { if (e.code !== 'MODULE_NOT_FOUND') throw e; } @@ -689,6 +693,7 @@ Module._findPath = function(request, paths, isMain) { const selfFilename = trySelf(paths, exts, isMain, trailingSlash, request); if (selfFilename) { + emitExperimentalWarning('Package name self resolution'); Module._pathCache[cacheKey] = selfFilename; return selfFilename; } diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 76bd3fe04e320d..2fa9d631ee2ec9 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -22,7 +22,7 @@ const createDynamicModule = require( const fs = require('fs'); const { fileURLToPath, URL } = require('url'); const { debuglog } = require('internal/util/debuglog'); -const { promisify } = require('internal/util'); +const { promisify, emitExperimentalWarning } = require('internal/util'); const { ERR_INVALID_URL, ERR_INVALID_URL_SCHEME, @@ -134,6 +134,7 @@ translators.set('builtin', async function builtinStrategy(url) { // Strategy for loading a JSON file translators.set('json', async function jsonStrategy(url) { + emitExperimentalWarning('Importing JSON modules'); debug(`Translating JSONModule ${url}`); debug(`Loading JSONModule ${url}`); const pathname = url.startsWith('file:') ? fileURLToPath(url) : null; @@ -188,6 +189,7 @@ translators.set('json', async function jsonStrategy(url) { // Strategy for loading a wasm module translators.set('wasm', async function(url) { + emitExperimentalWarning('Importing Web Assembly modules'); const buffer = await getSource(url); debug(`Translating WASMModule ${url}`); let compiled; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 3c3d568329221d..2fa8ba498edcb7 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -7,6 +7,7 @@ #include "util-inl.h" #include "node_contextify.h" #include "node_watchdog.h" +#include "node_process.h" #include // S_IFDIR @@ -962,6 +963,7 @@ Maybe ResolveExportsTarget(Environment* env, Maybe resolved = ResolveExportsTarget(env, pjson_url, conditionalTarget, subpath, pkg_subpath, base, false); if (!resolved.IsNothing()) { + ProcessEmitExperimentalWarning(env, "Conditional exports"); return resolved; } } @@ -1267,6 +1269,7 @@ Maybe PackageResolve(Environment* env, Maybe self_url = ResolveSelf(env, specifier, base); if (self_url.IsJust()) { + ProcessEmitExperimentalWarning(env, "Package name self resolution"); return self_url; } diff --git a/src/node_process.h b/src/node_process.h index 5db7b004d6f939..ad86b449f95290 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -27,6 +27,8 @@ v8::Maybe ProcessEmitWarningGeneric(Environment* env, const char* code = nullptr); v8::Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...); +v8::Maybe ProcessEmitExperimentalWarning(Environment* env, + const char* warning); v8::Maybe ProcessEmitDeprecationWarning(Environment* env, const char* warning, const char* deprecation_code); diff --git a/src/node_process_events.cc b/src/node_process_events.cc index 440e67d412322b..b090b6c24b3383 100644 --- a/src/node_process_events.cc +++ b/src/node_process_events.cc @@ -1,4 +1,5 @@ #include +#include #include "env-inl.h" #include "node_process.h" @@ -93,6 +94,21 @@ Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...) { return ProcessEmitWarningGeneric(env, warning); } + +std::set experimental_warnings; + +Maybe ProcessEmitExperimentalWarning(Environment* env, + const char* warning) { + if (experimental_warnings.find(warning) != experimental_warnings.end()) + return Nothing(); + + experimental_warnings.insert(warning); + std::string message(warning); + message.append( + " is an experimental feature. This feature could change at any time"); + return ProcessEmitWarningGeneric(env, message.c_str(), "ExperimentalWarning"); +} + Maybe ProcessEmitDeprecationWarning(Environment* env, const char* warning, const char* deprecation_code) { diff --git a/test/common/fixtures.mjs b/test/common/fixtures.mjs new file mode 100644 index 00000000000000..3eeef6d6077c4a --- /dev/null +++ b/test/common/fixtures.mjs @@ -0,0 +1,16 @@ +/* eslint-disable node-core/require-common-first, node-core/required-modules */ +import fixtures from './fixtures.js'; + +const { + fixturesDir, + path, + readSync, + readKey, +} = fixtures; + +export { + fixturesDir, + path, + readSync, + readKey, +}; diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 2683b5df68e9fa..8e2a843741daf4 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -1,7 +1,9 @@ // Flags: --experimental-modules --experimental-resolve-self --experimental-conditional-exports import { mustCall } from '../common/index.mjs'; +import { path } from '../common/fixtures.mjs'; import { ok, deepStrictEqual, strictEqual } from 'assert'; +import { spawn } from 'child_process'; import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; import fromInside from '../fixtures/node_modules/pkgexports/lib/hole.js'; @@ -150,3 +152,37 @@ function assertIncludes(actual, expected) { ok(actual.toString().indexOf(expected) !== -1, `${JSON.stringify(actual)} includes ${JSON.stringify(expected)}`); } + +// Test warning message +[ + [ + '--experimental-conditional-exports', + '/es-modules/conditional-exports.js', + 'Conditional exports', + ], + [ + '--experimental-resolve-self', + '/node_modules/pkgexports/resolve-self.js', + 'Package name self resolution', + ], +].forEach(([flag, file, message]) => { + const child = spawn(process.execPath, [ + '--experimental-modules', + flag, + path(file) + ]); + + let stderr = ''; + child.stderr.setEncoding('utf8'); + child.stderr.on('data', (data) => { + stderr += data; + }); + child.on('close', (code, signal) => { + strictEqual(code, 0); + strictEqual(signal, null); + ok(stderr.toString().includes( + `ExperimentalWarning: ${message} is an experimental feature. ` + + 'This feature could change at any time' + )); + }); +}); diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index 3d246124a9bdae..3f4608d09d9604 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -1,7 +1,30 @@ // Flags: --experimental-modules --experimental-json-modules import '../common/index.mjs'; -import { strictEqual } from 'assert'; +import { path } from '../common/fixtures.mjs'; +import { strictEqual, ok } from 'assert'; +import { spawn } from 'child_process'; import secret from '../fixtures/experimental.json'; strictEqual(secret.ofLife, 42); + +// Test warning message +const child = spawn(process.execPath, [ + '--experimental-modules', + '--experimental-json-modules', + path('/es-modules/json-modules.mjs') +]); + +let stderr = ''; +child.stderr.setEncoding('utf8'); +child.stderr.on('data', (data) => { + stderr += data; +}); +child.on('close', (code, signal) => { + strictEqual(code, 0); + strictEqual(signal, null); + ok(stderr.toString().includes( + 'ExperimentalWarning: Importing JSON modules is an experimental feature. ' + + 'This feature could change at any time' + )); +}); diff --git a/test/es-module/test-esm-wasm.mjs b/test/es-module/test-esm-wasm.mjs index bcfce797a9cc8b..4a9180199c95e7 100644 --- a/test/es-module/test-esm-wasm.mjs +++ b/test/es-module/test-esm-wasm.mjs @@ -1,8 +1,10 @@ // Flags: --experimental-modules --experimental-wasm-modules import '../common/index.mjs'; +import { path } from '../common/fixtures.mjs'; import { add, addImported } from '../fixtures/es-modules/simple.wasm'; import { state } from '../fixtures/es-modules/wasm-dep.mjs'; -import { strictEqual } from 'assert'; +import { strictEqual, ok } from 'assert'; +import { spawn } from 'child_process'; strictEqual(state, 'WASM Start Executed'); @@ -13,3 +15,24 @@ strictEqual(addImported(0), 42); strictEqual(state, 'WASM JS Function Executed'); strictEqual(addImported(1), 43); + +// Test warning message +const child = spawn(process.execPath, [ + '--experimental-modules', + '--experimental-wasm-modules', + path('/es-modules/wasm-modules.mjs') +]); + +let stderr = ''; +child.stderr.setEncoding('utf8'); +child.stderr.on('data', (data) => { + stderr += data; +}); +child.on('close', (code, signal) => { + strictEqual(code, 0); + strictEqual(signal, null); + ok(stderr.toString().includes( + 'ExperimentalWarning: Importing Web Assembly modules is ' + + 'an experimental feature. This feature could change at any time' + )); +}); diff --git a/test/fixtures/es-modules/conditional-exports.js b/test/fixtures/es-modules/conditional-exports.js new file mode 100644 index 00000000000000..b480078bfc47f1 --- /dev/null +++ b/test/fixtures/es-modules/conditional-exports.js @@ -0,0 +1 @@ +require('pkgexports/condition') diff --git a/test/fixtures/es-modules/json-modules.mjs b/test/fixtures/es-modules/json-modules.mjs new file mode 100644 index 00000000000000..fa3f936bac921e --- /dev/null +++ b/test/fixtures/es-modules/json-modules.mjs @@ -0,0 +1 @@ +import secret from '../experimental.json'; diff --git a/test/fixtures/es-modules/wasm-modules.mjs b/test/fixtures/es-modules/wasm-modules.mjs new file mode 100644 index 00000000000000..c56e6a926b2a04 --- /dev/null +++ b/test/fixtures/es-modules/wasm-modules.mjs @@ -0,0 +1,2 @@ +import { add, addImported } from './simple.wasm'; +import { state } from './wasm-dep.mjs'; diff --git a/test/fixtures/node_modules/pkgexports/resolve-self.js b/test/fixtures/node_modules/pkgexports/resolve-self.js new file mode 100644 index 00000000000000..7bd3526aaa4e70 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/resolve-self.js @@ -0,0 +1 @@ +require('@pkgexports/name/valid-cjs')