From 828d5d22eb8636cd8458af04aabd6b9972c16875 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 11 Mar 2020 10:38:00 -0500 Subject: [PATCH] esm: doc & validate source values for formats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/32202 Reviewed-By: Guy Bedford Reviewed-By: Geoffrey Booth Reviewed-By: Michaël Zasso Reviewed-By: Zeyu Yang --- doc/api/esm.md | 35 +++++++++---- lib/internal/modules/esm/translators.js | 43 ++++++++++++++-- .../test-esm-loader-stringify-text.mjs | 50 +++++++++++++++++++ .../es-module-loaders/string-sources.mjs | 30 +++++++++++ .../es-module-loaders/transform-source.mjs | 5 +- 5 files changed, 150 insertions(+), 13 deletions(-) create mode 100644 test/es-module/test-esm-loader-stringify-text.mjs create mode 100644 test/fixtures/es-module-loaders/string-sources.mjs diff --git a/doc/api/esm.md b/doc/api/esm.md index e5c957d03ce6a0..8a3d5bdecbdb2e 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1196,16 +1196,27 @@ export async function resolve(specifier, context, defaultResolve) { > signature may change. Do not rely on the API described below. The `getFormat` hook provides a way to define a custom method of determining how -a URL should be interpreted. This can be one of the following: +a URL should be interpreted. The `format` returned also affects what the +acceptable forms of source values are for a module when parsing. This can be one +of the following: -| `format` | Description | -| --- | --- | -| `'builtin'` | Load a Node.js builtin module | -| `'commonjs'` | Load a Node.js CommonJS module | -| `'dynamic'` | Use a [dynamic instantiate hook][] | -| `'json'` | Load a JSON file | -| `'module'` | Load a standard JavaScript module (ES module) | -| `'wasm'` | Load a WebAssembly module | +| `format` | Description | Acceptable Types For `source` Returned by `getSource` or `transformSource` | +| --- | --- | --- | +| `'builtin'` | Load a Node.js builtin module | Not applicable | +| `'commonjs'` | Load a Node.js CommonJS module | Not applicable | +| `'dynamic'` | Use a [dynamic instantiate hook][] | Not applicable | +| `'json'` | Load a JSON file | { [ArrayBuffer][], [string][], [TypedArray][] } | +| `'module'` | Load an ES module | { [ArrayBuffer][], [string][], [TypedArray][] } | +| `'wasm'` | Load a WebAssembly module | { [ArrayBuffer][], [string][], [TypedArray][] } | + +Note: These types all correspond to classes defined in ECMAScript. + +* The specific [ArrayBuffer][] object is a [SharedArrayBuffer][]. +* The specific [string][] object is not the class constructor, but an instance. +* The specific [TypedArray][] object is a [Uint8Array][]. + +Note: If the source value of a text-based format (i.e., `'json'`, `'module'`) is +not a string, it will be converted to a string using [`util.TextDecoder`][]. ```js /** @@ -1842,6 +1853,12 @@ success! [`module.createRequire()`]: modules.html#modules_module_createrequire_filename [`module.syncBuiltinESMExports()`]: modules.html#modules_module_syncbuiltinesmexports [`transformSource` hook]: #esm_code_transformsource_code_hook +[ArrayBuffer]: http://www.ecma-international.org/ecma-262/6.0/#sec-arraybuffer-constructor +[SharedArrayBuffer]: https://tc39.es/ecma262/#sec-sharedarraybuffer-constructor +[string]: http://www.ecma-international.org/ecma-262/6.0/#sec-string-constructor +[TypedArray]: http://www.ecma-international.org/ecma-262/6.0/#sec-typedarray-objects +[Uint8Array]: http://www.ecma-international.org/ecma-262/6.0/#sec-uint8array +[`util.TextDecoder`]: util.html#util_class_util_textdecoder [dynamic instantiate hook]: #esm_code_dynamicinstantiate_code_hook [import an ES or CommonJS module for its side effects only]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Import_a_module_for_its_side_effects_only [special scheme]: https://url.spec.whatwg.org/#special-scheme diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index a7252c1c99f954..f314ba96b3476c 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,6 +11,12 @@ const { StringPrototypeReplace, } = primordials; +let _TYPES = null; +function lazyTypes() { + if (_TYPES !== null) return _TYPES; + return _TYPES = require('internal/util/types'); +} + const { stripBOM, loadNativeModule @@ -26,7 +32,10 @@ const createDynamicModule = require( const { fileURLToPath, URL } = require('url'); const { debuglog } = require('internal/util/debuglog'); const { emitExperimentalWarning } = require('internal/util'); -const { ERR_UNKNOWN_BUILTIN_MODULE } = require('internal/errors').codes; +const { + ERR_UNKNOWN_BUILTIN_MODULE, + ERR_INVALID_RETURN_PROPERTY_VALUE +} = require('internal/errors').codes; const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache'); const moduleWrap = internalBinding('module_wrap'); const { ModuleWrap } = moduleWrap; @@ -39,6 +48,30 @@ const debug = debuglog('esm'); const translators = new SafeMap(); exports.translators = translators; +let DECODER = null; +function assertBufferSource(body, allowString, hookName) { + if (allowString && typeof body === 'string') { + return; + } + const { isArrayBufferView, isAnyArrayBuffer } = lazyTypes(); + if (isArrayBufferView(body) || isAnyArrayBuffer(body)) { + return; + } + throw new ERR_INVALID_RETURN_PROPERTY_VALUE( + `${allowString ? 'string, ' : ''}array buffer, or typed array`, + hookName, + 'source', + body + ); +} + +function stringify(body) { + if (typeof body === 'string') return body; + assertBufferSource(body, false, 'transformSource'); + DECODER = DECODER === null ? new TextDecoder() : DECODER; + return DECODER.decode(body); +} + function errPath(url) { const parsed = new URL(url); if (parsed.protocol === 'file:') { @@ -80,9 +113,10 @@ function initializeImportMeta(meta, { url }) { translators.set('module', async function moduleStrategy(url) { let { source } = await this._getSource( url, { format: 'module' }, defaultGetSource); - source = `${source}`; + assertBufferSource(source, true, 'getSource'); ({ source } = await this._transformSource( source, { url, format: 'module' }, defaultTransformSource)); + source = stringify(source); maybeCacheSourceMap(url, source); debug(`Translating StandardModule ${url}`); const module = new ModuleWrap(url, undefined, source, 0, 0); @@ -157,9 +191,10 @@ translators.set('json', async function jsonStrategy(url) { } let { source } = await this._getSource( url, { format: 'json' }, defaultGetSource); - source = `${source}`; + assertBufferSource(source, true, 'getSource'); ({ source } = await this._transformSource( source, { url, format: 'json' }, defaultTransformSource)); + source = stringify(source); if (pathname) { // A require call could have been called on the same file during loading and // that resolves synchronously. To make sure we always return the identical @@ -200,8 +235,10 @@ translators.set('wasm', async function(url) { emitExperimentalWarning('Importing Web Assembly modules'); let { source } = await this._getSource( url, { format: 'wasm' }, defaultGetSource); + assertBufferSource(source, false, 'getSource'); ({ source } = await this._transformSource( source, { url, format: 'wasm' }, defaultTransformSource)); + assertBufferSource(source, false, 'transformSource'); debug(`Translating WASMModule ${url}`); let compiled; try { diff --git a/test/es-module/test-esm-loader-stringify-text.mjs b/test/es-module/test-esm-loader-stringify-text.mjs new file mode 100644 index 00000000000000..ed9bd5069ebf1d --- /dev/null +++ b/test/es-module/test-esm-loader-stringify-text.mjs @@ -0,0 +1,50 @@ +// Flags: --experimental-loader ./test/fixtures/es-module-loaders/string-sources.mjs +import { mustCall, mustNotCall } from '../common/index.mjs'; +import assert from 'assert'; + +import('test:Array').then( + mustNotCall('Should not accept Arrays'), + mustCall((e) => { + assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); + }) +); +import('test:ArrayBuffer').then( + mustCall(), + mustNotCall('Should accept ArrayBuffers'), +); +import('test:null').then( + mustNotCall('Should not accept null'), + mustCall((e) => { + assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); + }) +); +import('test:Object').then( + mustNotCall('Should not stringify or valueOf Objects'), + mustCall((e) => { + assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); + }) +); +import('test:SharedArrayBuffer').then( + mustCall(), + mustNotCall('Should accept SharedArrayBuffers'), +); +import('test:string').then( + mustCall(), + mustNotCall('Should accept strings'), +); +import('test:String').then( + mustNotCall('Should not accept wrapper Strings'), + mustCall((e) => { + assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); + }) +); +import('test:Uint8Array').then( + mustCall(), + mustNotCall('Should accept Uint8Arrays'), +); +import('test:undefined').then( + mustNotCall('Should not accept undefined'), + mustCall((e) => { + assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); + }) +); diff --git a/test/fixtures/es-module-loaders/string-sources.mjs b/test/fixtures/es-module-loaders/string-sources.mjs new file mode 100644 index 00000000000000..3133231208ce1e --- /dev/null +++ b/test/fixtures/es-module-loaders/string-sources.mjs @@ -0,0 +1,30 @@ +const SOURCES = { + __proto__: null, + 'test:Array': ['1', '2'], // both `1,2` and `12` are valid ESM + 'test:ArrayBuffer': new ArrayBuffer(0), + 'test:null': null, + 'test:Object': {}, + 'test:SharedArrayBuffer': new SharedArrayBuffer(0), + 'test:string': '', + 'test:String': new String(''), + 'test:Uint8Array': new Uint8Array(0), + 'test:undefined': undefined, +} +export function resolve(specifier, context, defaultFn) { + if (specifier.startsWith('test:')) { + return { url: specifier }; + } + return defaultFn(specifier, context); +} +export function getFormat(href, context, defaultFn) { + if (href.startsWith('test:')) { + return { format: 'module' }; + } + return defaultFn(href, context); +} +export function getSource(href, context, defaultFn) { + if (href.startsWith('test:')) { + return { source: SOURCES[href] }; + } + return defaultFn(href, context); +} diff --git a/test/fixtures/es-module-loaders/transform-source.mjs b/test/fixtures/es-module-loaders/transform-source.mjs index ab147c34cb34fd..25d983b64e62ca 100644 --- a/test/fixtures/es-module-loaders/transform-source.mjs +++ b/test/fixtures/es-module-loaders/transform-source.mjs @@ -1,6 +1,9 @@ export async function transformSource( source, { url, format }, defaultTransformSource) { - if (source && source.replace) { + if (format === 'module') { + if (typeof source !== 'string') { + source = new TextDecoder().decode(source); + } return { source: source.replace(`'A message';`, `'A message'.toUpperCase();`) };