Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ERR_REQUIRE_ESM when a file is require()d which should be loaded as ESM #1031

Merged
merged 18 commits into from May 20, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions .github/workflows/continuous-integration.yml
Expand Up @@ -39,27 +39,33 @@ jobs:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest]
flavor: [1, 2, 3, 4, 5, 6, 7]
flavor: [1, 2, 3, 4, 5, 6, 7, 8, 9]
include:
- flavor: 1
node: 6
typescript: typescript@latest
- flavor: 2
node: 13
node: 12.15
typescript: typescript@latest
- flavor: 3
node: 12.16
typescript: typescript@latest
- flavor: 4
node: 13
typescript: typescript@latest
- flavor: 5
node: 13
typescript: typescript@2.7
- flavor: 4
- flavor: 6
node: 13
typescript: typescript@next
- flavor: 5
- flavor: 7
node: 14
typescript: typescript@latest
- flavor: 6
- flavor: 8
node: 14
typescript: typescript@2.7
- flavor: 7
- flavor: 9
node: 14
typescript: typescript@next
steps:
Expand Down
118 changes: 118 additions & 0 deletions dist-raw/node-cjs-loader-utils.js
@@ -0,0 +1,118 @@
// copied from https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this whole file copied or which parts should be reviewed? Curious on things like we read JSON ourselves instead of relying on the require cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this is actually coming from several files in node's source. I added comments to each item in this file with a permalink to the span of node's source code on github.

const path = require('path');
const fs = require('fs');

module.exports.assertScriptCanLoadAsCJSImpl = assertScriptCanLoadAsCJSImpl;

// copied from Module._extensions['.js']
function assertScriptCanLoadAsCJSImpl(filename) {
const pkg = readPackageScope(filename);
// Function require shouldn't be used in ES modules.
if (pkg && pkg.data && pkg.data.type === 'module') {
const parentPath = module.parent && module.parent.filename;
const packageJsonPath = path.resolve(pkg.path, 'package.json');
throw createErrRequireEsm(filename, parentPath, packageJsonPath);
}
}

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 {
path: checkPath,
data: pjson
};
}
return false;
}

const packageJsonCache = new Map();

function readPackage(requestPath) {
const jsonPath = path.resolve(requestPath, 'package.json');

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;
}

// TODO Related to `--experimental-policy`? Disabling for now
// if (manifest) {
// const jsonURL = pathToFileURL(jsonPath);
// manifest.assertIntegrity(jsonURL, json);
// }

try {
const parsed = JSON.parse(json);
const filtered = {
name: parsed.name,
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;
throw e;
}
}

function internalModuleReadJSON(path) {
try {
return fs.readFileSync(path, 'utf8')
} catch (e) {
if (e.code === 'ENOENT') return undefined
throw e
}
}

function createErrRequireEsm(filename, parentPath, packageJsonPath) {
// Attempt to create an error object that matches node's native error close enough
const code = 'ERR_REQUIRE_ESM'
const err = new Error(getMessage(filename, parentPath, packageJsonPath))
err.name = `Error [${ code }]`
err.stack
Object.defineProperty(err, 'name', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty funky, might want to add a comment on why you do this since the first look makes it seem this line would erase err.name = above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

value: 'Error',
enumerable: false,
writable: true,
configurable: true
})
err.code = code
return err

// copy-pasted from https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L1294-L1311
function getMessage(filename, parentPath = null, packageJsonPath = null) {
const ext = path.extname(filename)
let msg = `Must use import to load ES Module: ${filename}`;
if (parentPath && packageJsonPath) {
const path = require('path');
const basename = path.basename(filename) === path.basename(parentPath) ?
filename : path.basename(filename);
msg +=
'\nrequire() of ES modules is not supported.\nrequire() of ' +
`${filename} ${parentPath ? `from ${parentPath} ` : ''}` +
`is an ES module file as it is a ${ext} file whose nearest parent ` +
`package.json contains "type": "module" which defines all ${ext} ` +
'files in that package scope as ES modules.\nInstead ' +
'change the requiring code to use ' +
'import(), or remove "type": "module" from ' +
`${packageJsonPath}.\n`;
return msg;
}
return msg;
}
}
33 changes: 33 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Expand Up @@ -65,6 +65,7 @@
"@types/node": "13.13.5",
"@types/proxyquire": "^1.3.28",
"@types/react": "^16.0.2",
"@types/rimraf": "^3.0.0",
"@types/semver": "^7.1.0",
"@types/source-map-support": "^0.5.0",
"axios": "^0.19.0",
Expand Down
5 changes: 4 additions & 1 deletion src/esm.ts
Expand Up @@ -8,7 +8,10 @@ const { createResolve } = require('../dist-raw/node-esm-resolve-implementation')

export function registerAndCreateEsmHooks (opts?: RegisterOptions) {
// Automatically performs registration just like `-r ts-node/register`
const tsNodeInstance = register(opts)
const tsNodeInstance = register({
...opts,
experimentalEsmLoader: true
})

// Custom implementation that considers additional file extensions and automatically adds file extensions
const nodeResolveImplementation = createResolve({
Expand Down
29 changes: 24 additions & 5 deletions src/index.spec.ts
Expand Up @@ -7,6 +7,7 @@ import proxyquire = require('proxyquire')
import { register, create, VERSION } from './index'
import { unlinkSync, existsSync, statSync } from 'fs'
import * as promisify from 'util.promisify'
import { sync as rimrafSync } from 'rimraf'

const execP = promisify(exec)

Expand All @@ -20,6 +21,7 @@ const SOURCE_MAP_REGEXP = /\/\/# sourceMappingURL=data:application\/json;charset
// Pack and install ts-node locally, necessary to test package "exports"
before(async function () {
this.timeout(30000)
rimrafSync(join(TEST_DIR, 'node_modules'))
await execP(`npm install`, { cwd: TEST_DIR })
const packageLockPath = join(TEST_DIR, 'package-lock.json')
existsSync(packageLockPath) && unlinkSync(packageLockPath)
Expand Down Expand Up @@ -678,12 +680,12 @@ describe('ts-node', function () {
})
})

if (semver.gte(process.version, '13.0.0')) {
describe('esm', () => {
this.slow(1000)
describe('esm', () => {
this.slow(1000)

const cmd = `node --loader ../../esm.mjs`
const cmd = `node --loader ts-node/esm.mjs`

if (semver.gte(process.version, '13.0.0')) {
it('should compile and execute as ESM', (done) => {
exec(`${cmd} index.ts`, { cwd: join(__dirname, '../tests/esm') }, function (err, stdout) {
expect(err).to.equal(null)
Expand All @@ -701,6 +703,23 @@ describe('ts-node', function () {
})

})
it('throws ERR_REQUIRE_ESM when attempting to require() an ESM script while ESM loader is enabled', function (done) {
exec(`${cmd} ./index.js`, { cwd: join(__dirname, '../tests/esm-err-require-esm') }, function (err, stdout, stderr) {
expect(err).to.not.equal(null)
expect(stderr).to.contain('Error [ERR_REQUIRE_ESM]: Must use import to load ES Module:')

return done()
})
})
}

it('executes ESM as CJS, ignoring package.json "types" field (for backwards compatibility; should be changed in next major release to throw ERR_REQUIRE_ESM)', function (done) {
exec(`${BIN_PATH} ./tests/esm-err-require-esm/index.js`, function (err, stdout) {
expect(err).to.equal(null)
expect(stdout).to.equal('CommonJS\n')

return done()
})
})
}
})
})
35 changes: 31 additions & 4 deletions src/index.ts
Expand Up @@ -5,6 +5,25 @@ import { BaseError } from 'make-error'
import * as util from 'util'
import * as _ts from 'typescript'

/**
* Does this version of node obey the package.json "type" field
* and attempt to load scripts as ESM
*/
const engineSupportsPackageTypeField = parseInt(process.versions.node.split('.')[0], 10) >= 12

// Loaded conditionally so we don't need to support older node versions
let assertScriptCanLoadAsCJSImpl: ((filename: string) => void) | undefined

/**
* Assert that script can be loaded as CommonJS when we attempt to require it.
* If it should be loaded as ESM, throw ERR_REQUIRE_ESM like node does.
*/
function assertScriptCanLoadAsCJS (filename: string) {
if (!engineSupportsPackageTypeField) return
if (!assertScriptCanLoadAsCJSImpl) assertScriptCanLoadAsCJSImpl = require('../dist-raw/node-cjs-loader-utils').assertScriptCanLoadAsCJSImpl
assertScriptCanLoadAsCJSImpl!(filename)
}

/**
* Registered `ts-node` instance information.
*/
Expand Down Expand Up @@ -179,6 +198,12 @@ export interface CreateOptions {
readFile?: (path: string) => string | undefined
fileExists?: (path: string) => boolean
transformers?: _ts.CustomTransformers | ((p: _ts.Program) => _ts.CustomTransformers)
/**
* True if require() hooks should interop with experimental ESM loader.
* Enabled explicitly via a flag since it is a breaking change.
* @internal
*/
experimentalEsmLoader?: boolean
}

/**
Expand Down Expand Up @@ -247,7 +272,8 @@ export const DEFAULTS: RegisterOptions = {
transpileOnly: yn(process.env.TS_NODE_TRANSPILE_ONLY),
typeCheck: yn(process.env.TS_NODE_TYPE_CHECK),
compilerHost: yn(process.env.TS_NODE_COMPILER_HOST),
logError: yn(process.env.TS_NODE_LOG_ERROR)
logError: yn(process.env.TS_NODE_LOG_ERROR),
experimentalEsmLoader: false
}

/**
Expand Down Expand Up @@ -770,9 +796,6 @@ export function create (rawOptions: CreateOptions = {}): Register {
}
}

const cannotCompileViaBothCodepathsErrorMessage = 'Cannot compile the same file via both `require()` and ESM hooks codepaths. ' +
'This breaks source-map-support, which cannot tell the difference between the two sourcemaps. ' +
'To avoid this problem, load each .ts file as only ESM or only CommonJS.'
// Create a simple TypeScript compiler proxy.
function compile (code: string, fileName: string, lineOffset = 0) {
const normalizedFileName = normalizeSlashes(fileName)
Expand Down Expand Up @@ -854,6 +877,10 @@ function registerExtension (
require.extensions[ext] = function (m: any, filename) { // tslint:disable-line
if (register.ignored(filename)) return old(m, filename)

if (register.options.experimentalEsmLoader) {
assertScriptCanLoadAsCJS(filename)
}

const _compile = m._compile

m._compile = function (code: string, fileName: string) {
Expand Down
5 changes: 5 additions & 0 deletions tests/esm-err-require-esm/esm-package/loaded-as.ts
@@ -0,0 +1,5 @@
// Log if this file is loaded as ESM or CommonJS
if(typeof module !== 'undefined')
console.log('CommonJS')
else
console.log('ESM')
3 changes: 3 additions & 0 deletions tests/esm-err-require-esm/esm-package/package.json
@@ -0,0 +1,3 @@
{
"type": "module"
}
1 change: 1 addition & 0 deletions tests/esm-err-require-esm/index.js
@@ -0,0 +1 @@
require('./esm-package/loaded-as')