From 851f3135aba0fdbf0fc89f1ef346adf438f4c399 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 | 32 +++++++++++++++++++ test/es-module/test-esm-json.mjs | 24 +++++++++++++- test/es-module/test-esm-wasm.mjs | 24 +++++++++++++- .../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, 133 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 d962bc2a4cec30..11e3d59f781dc3 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -43,7 +43,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'); @@ -584,8 +584,10 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (experimentalConditionalExports && ObjectPrototypeHasOwnProperty(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; } @@ -593,8 +595,10 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (experimentalConditionalExports && ObjectPrototypeHasOwnProperty(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; } @@ -697,6 +701,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 dafc1b02f03112..99e4c014053202 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, @@ -133,6 +133,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; @@ -187,6 +188,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 06096226625bb6..d192ef19b7abad 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" @@ -95,6 +96,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 a361bafaa83804..73a86793a23863 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -1,6 +1,8 @@ // Flags: --experimental-modules 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'; @@ -149,3 +151,33 @@ 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, [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 9dd33f26b557d1..ddcc7cb33e709f 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -1,7 +1,29 @@ // Flags: --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-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 46df5994afa932..b2218ce2f09893 100644 --- a/test/es-module/test-esm-wasm.mjs +++ b/test/es-module/test-esm-wasm.mjs @@ -1,8 +1,10 @@ // Flags: --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,23 @@ strictEqual(addImported(0), 42); strictEqual(state, 'WASM JS Function Executed'); strictEqual(addImported(1), 43); + +// Test warning message +const child = spawn(process.execPath, [ + '--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')