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

Inconsistent behaviour with moduleResolution: "node16" and swc: true for CommonJS module output #2035

Open
jdb8 opened this issue Jul 14, 2023 · 2 comments

Comments

@jdb8
Copy link

jdb8 commented Jul 14, 2023

I decided to file this issue here after looking at both the swc and ts-node issues - apologies if this is a better fit for swc! I'm also fully expecting to be told that there is additional swc configuration required, in which case this issue could be reframed as "add additional info on configuring swc to https://typestrong.org/ts-node/docs/swc/".

Search Terms

moduleResolution, swc, CommonJS, ERR_REQUIRE_ESM, import()

Expected Behavior

In a TypeScript project outputting CommonJS via "module": "CommonJS", ts-node should compile and run files the same regardless of the swc option. See below for an example where this surprisingly wasn't the case.

This came up when importing an ESM-only package into a TypeScript file targeting CommonJS. Node gives us the suggestion to Instead change the require of index.js in /Users/jbateson/code/ts-node-swc-module-resolution-repro/index.ts to a dynamic import() which is available in all CommonJS modules., but this doesn't seem to work with swc: true.

Actual Behavior

Given the following tsconfig.json:

{
    "compilerOptions": {
        "target": "es2021",
        "module": "CommonJS",
        "moduleResolution": "node16",
        "esModuleInterop": true
    }
}

the following code combining static ESM imports + dynamic imports of ESM-only code works fine:

import fs from 'fs';

(async function main() {
    console.log('main');

    // Import node-fetch dynamically as it's pure ESM, and our output target is CJS
    // @ts-ignore
    const fetch = await import('node-fetch');

    console.log({ fetch, fs });
})();

but when adding

    "ts-node": {
        "swc": true
    }

it outputs the following when invoked as ts-node index.ts:

❯ ts-node index.ts
main
/Users/jbateson/code/ts-node-swc-module-resolution-repro/node_modules/ts-node/dist/index.js:851
            return old(m, filename);
                   ^
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/jbateson/code/ts-node-swc-module-resolution-repro/node_modules/node-fetch/src/index.js from /Users/jbateson/code/ts-node-swc-module-resolution-repro/index.ts not supported.
Instead change the require of index.js in /Users/jbateson/code/ts-node-swc-module-resolution-repro/index.ts to a dynamic import() which is available in all CommonJS modules.
    at Object.require.extensions.<computed> [as .js] (/Users/jbateson/code/ts-node-swc-module-resolution-repro/node_modules/ts-node/dist/index.js:851:20)
    at /Users/jbateson/code/ts-node-swc-module-resolution-repro/index.ts:54:92
    at async main (/Users/jbateson/code/ts-node-swc-module-resolution-repro/index.ts:54:19) {
  code: 'ERR_REQUIRE_ESM'
}

which implies that swc is transpiling the import() differently from the default ts-node/tsc behaviour.

I've "verified" this (kinda) by looking at the swc repl, which turns import() into require() unconditionally when the target is CommonJS.

Steps to reproduce the problem

See above, also see a link to a runnable version of the same minimal repro below.

Minimal reproduction

Clone this repo, run yarn to install dependencies, and then run ts-node index.ts to reproduce the above: https://github.com/jdb8/ts-node-swc-module-resolution-repro

Commenting out swc: true will show the expected behaviour.

Specifications

  • ts-node version: v10.9.1
  • node version: v16.16.0
  • TypeScript version: v5.1.6
  • tsconfig.json, if you're using one:
{
    "compilerOptions": {
        "target": "es2021",
        "module": "CommonJS",
        "moduleResolution": "node16",
        "esModuleInterop": true
    },
    "ts-node": {
        "swc": true
    }
}
  • package.json:
{
  "name": "ts-node-swc-module-resolution-repro",
  "version": "1.0.0",
  "main": "index.js",
  "author": "Joe Bateson <[email protected]>",
  "license": "MIT",
  "dependencies": {
    "@swc/core": "^1.3.69",
    "@swc/helpers": "^0.5.1",
    "node-fetch": "^3.3.1",
    "ts-node": "^10.9.1",
    "typescript": "^5.1.6"
  },
  "devDependencies": {
    "@types/node": "^20.4.2"
  }
}
  • Operating system and version: mac osx 13.4
  • If Windows, are you using WSL or WSL2?: n/a
@jdb8
Copy link
Author

jdb8 commented Jul 14, 2023

Looking at the swc side of things, it seems like the "import() gets transpiled to require()" issue can be fixed by setting ignoreDynamic in .swcrc: https://swc.rs/docs/configuration/modules#ignoredynamic

But unfortunately due to #1856 it looks like that doesn't fix the issue when running the code via ts-node.

Back here, looking at the internals of the swc transpiler, it seems like this case is accounted for:

ts-node/src/index.ts

Lines 1469 to 1474 in 47d4f45

* When using `module: nodenext` or `module: node12`, there are two possible styles of emit depending in file extension or package.json "type":
*
* - CommonJS with dynamic imports preserved (not transformed into `require()` calls)
* - ECMAScript modules with `import foo = require()` transformed into `require = createRequire(); const foo = require()`
*/
export type NodeModuleEmitKind = 'nodeesm' | 'nodecjs';

But so far it's not obvious to me how to force ts-node to enter the ignoreDynamic codepath here:

ignoreDynamic: nodeModuleEmitKind === 'nodecjs',

(i.e. a way to force ts-node to recognise the nodeModuleEmitKind as nodecjs)

@jdb8
Copy link
Author

jdb8 commented Jul 14, 2023

Final findings for today, which may indicate a fix for my project (and highlight my configuration mistakes):

My confusion was assuming that module: commonjs was the recommended setting to emit commonjs, and I'd missed the fact that there was a separate "commonjs with ESM" flavour available.

I'm going to go back and update my non-toy project with these changes to see if that fixes things, but I assume it will. Assuming my understanding of the above is correct, this issue can probably be closed, but if possible it would be great if the docs could point to this behaviour somewhere, if others are using the stale commonjs module target where they instead should be using node16.

Happy to create a new issue for the doc updates if that's preferred!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant