Skip to content

Commit

Permalink
module: fix discrepancy between .ts and .js
Browse files Browse the repository at this point in the history
PR-URL: #54461
Fixes: #54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
  • Loading branch information
marco-ippolito committed Sep 16, 2024
1 parent a2a1ea2 commit dadd466
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 56 deletions.
14 changes: 7 additions & 7 deletions lib/internal/modules/esm/get_format.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,14 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
default: { // The user did not pass `--experimental-default-type`.
// `source` is undefined when this is called from `defaultResolve`;
// but this gets called again from `defaultLoad`/`defaultLoadSync`.
let parsedSource;
if (source) {
const { stripTypeScriptTypes } = require('internal/modules/helpers');
parsedSource = stripTypeScriptTypes(source, url);
}
// Since experimental-strip-types depends on detect-module, we always return null
// if source is undefined.
if (!source) { return null; }
const { stripTypeScriptTypes, stringify } = require('internal/modules/helpers');
const stringifiedSource = stringify(source);
const parsedSource = stripTypeScriptTypes(stringifiedSource, fileURLToPath(url));
const detectedFormat = detectModuleFormat(parsedSource, url);
// When source is undefined, default to module-typescript.
const format = detectedFormat ? `${detectedFormat}-typescript` : 'module-typescript';
const format = `${detectedFormat}-typescript`;
if (format === 'module-typescript' && foundPackageJson) {
// This module has a .js extension, a package.json with no `type` field, and ESM syntax.
// Warn about the missing `type` field so that the user can avoid the performance penalty of detection.
Expand Down
51 changes: 2 additions & 49 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,6 @@ const {
globalThis: { WebAssembly },
} = primordials;

/** @type {import('internal/util/types')} */
let _TYPES = null;
/**
* Lazily loads and returns the internal/util/types module.
*/
function lazyTypes() {
if (_TYPES !== null) { return _TYPES; }
return _TYPES = require('internal/util/types');
}

const {
compileFunctionForCJSLoader,
} = internalBinding('contextify');
Expand All @@ -37,7 +27,9 @@ const assert = require('internal/assert');
const { readFileSync } = require('fs');
const { dirname, extname, isAbsolute } = require('path');
const {
assertBufferSource,
loadBuiltinModule,
stringify,
stripTypeScriptTypes,
stripBOM,
urlToFilename,
Expand All @@ -57,7 +49,6 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
const { emitExperimentalWarning, kEmptyObject, setOwnProperty, isWindows } = require('internal/util');
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');
Expand Down Expand Up @@ -107,44 +98,6 @@ function initCJSParseSync() {
const translators = new SafeMap();
exports.translators = translators;

let DECODER = null;
/**
* Asserts that the given body is a buffer source (either a string, array buffer, or typed array).
* Throws an error if the body is not a buffer source.
* @param {string | ArrayBufferView | ArrayBuffer} body - The body to check.
* @param {boolean} allowString - Whether or not to allow a string as a valid buffer source.
* @param {string} hookName - The name of the hook being called.
* @throws {ERR_INVALID_RETURN_PROPERTY_VALUE} If the body is not a buffer source.
*/
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,
);
}

/**
* Converts a buffer or buffer-like object to a string.
* @param {string | ArrayBuffer | ArrayBufferView} body - The buffer or buffer-like object to convert to a string.
* @returns {string} The resulting string.
*/
function stringify(body) {
if (typeof body === 'string') { return body; }
assertBufferSource(body, false, 'load');
const { TextDecoder } = require('internal/encoding');
DECODER = DECODER === null ? new TextDecoder() : DECODER;
return DECODER.decode(body);
}

/**
* Converts a URL to a file path if the URL protocol is 'file:'.
* @param {string} url - The URL to convert.
Expand Down
52 changes: 52 additions & 0 deletions lib/internal/modules/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const {
} = primordials;
const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_RETURN_PROPERTY_VALUE,
} = require('internal/errors').codes;
const { BuiltinModule } = require('internal/bootstrap/realm');

Expand Down Expand Up @@ -429,10 +430,60 @@ function getCompileCacheDir() {
return _getCompileCacheDir() || undefined;
}

/** @type {import('internal/util/types')} */
let _TYPES = null;
/**
* Lazily loads and returns the internal/util/types module.
*/
function lazyTypes() {
if (_TYPES !== null) { return _TYPES; }
return _TYPES = require('internal/util/types');
}

/**
* Asserts that the given body is a buffer source (either a string, array buffer, or typed array).
* Throws an error if the body is not a buffer source.
* @param {string | ArrayBufferView | ArrayBuffer} body - The body to check.
* @param {boolean} allowString - Whether or not to allow a string as a valid buffer source.
* @param {string} hookName - The name of the hook being called.
* @throws {ERR_INVALID_RETURN_PROPERTY_VALUE} If the body is not a buffer source.
*/
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,
);
}

let DECODER = null;

/**
* Converts a buffer or buffer-like object to a string.
* @param {string | ArrayBuffer | ArrayBufferView} body - The buffer or buffer-like object to convert to a string.
* @returns {string} The resulting string.
*/
function stringify(body) {
if (typeof body === 'string') { return body; }
assertBufferSource(body, false, 'load');
const { TextDecoder } = require('internal/encoding');
DECODER = DECODER === null ? new TextDecoder() : DECODER;
return DECODER.decode(body);
}

module.exports = {
addBuiltinLibsToObject,
constants,
enableCompileCache,
assertBufferSource,
getBuiltinModule,
getCjsConditions,
getCompileCacheDir,
Expand All @@ -442,6 +493,7 @@ module.exports = {
makeRequireFunction,
normalizeReferrerURL,
stripTypeScriptTypes,
stringify,
stripBOM,
toRealPath,
hasStartedUserCJSExecution() {
Expand Down
11 changes: 11 additions & 0 deletions test/es-module/test-typescript.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,14 @@ test('execute a TypeScript file with CommonJS syntax requiring .mts with require
match(result.stdout, /Hello, TypeScript!/);
strictEqual(result.code, 0);
});

test('execute a JavaScript file importing a cjs TypeScript file', async () => {
const result = await spawnPromisified(process.execPath, [
'--experimental-strip-types',
'--no-warnings',
fixtures.path('typescript/ts/issue-54457.mjs'),
]);
strictEqual(result.stderr, '');
match(result.stdout, /Hello, TypeScript!/);
strictEqual(result.code, 0);
});
4 changes: 4 additions & 0 deletions test/fixtures/typescript/ts/issue-54457.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// https://github.com/nodejs/node/issues/54457
const { text } = await import('./test-require.ts');

console.log(text);
7 changes: 7 additions & 0 deletions test/fixtures/typescript/ts/test-require.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const util = require('node:util');

const text = 'Hello, TypeScript!';

module.exports = {
text
};

0 comments on commit dadd466

Please sign in to comment.