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

Does it make sense to allow module: nodenext + moduleResolution: node? #48854

Closed
DanielRosenwasser opened this issue Apr 26, 2022 · 4 comments
Closed
Assignees
Labels
Discussion Issues which may not have code impact

Comments

@DanielRosenwasser
Copy link
Member

In #48835, a user ran into an issue where the combined options were questionable. It seems like an innocent mistake, and by all means moduleResolution: node sounds compatible with module: nodenext or moduleResolution: node.

Should we issue an error in some of these cases?

CC @weswigham @andrewbranch

@weswigham
Copy link
Member

So it is technically a useful configuration - we won't detect any esm files (since the resolution mode is pre-esm), but we will use the new cjs transform (ie, leaving dynamic import alone) on all the input modules.

@andrewbranch andrewbranch added the Discussion Issues which may not have code impact label Apr 27, 2022
@craigphicks
Copy link

craigphicks commented May 5, 2022

To make a long story short

module moduleResolution uses package.json:type
unset "node12" true
"node12" unset true
"node12" "node" false

If you had a simple table like that (possibly with more entries) next to the documentation for both module and moduleResolution, I think it would prevent unnecessary confusion.

That is apart from the question of whether that combination should be allowed

@magicdawn
Copy link

the moduleResolution option naming is bad, classic & node is well documented on page https://www.typescriptlang.org/docs/handbook/module-resolution.html

but not node12/nodenext(now node16/nodenext), and not updated on tsconfig reference page

image

the node12 / node16 / nodenext should be renamed to node-esm and points a link to the Node.js ESM resolve spec https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#resolver-algorithm-specification

@andrewbranch
Copy link
Member

Strongly disagree. The TSConfig reference docs should change. node16/nodenext is the only correct module resolution option if you are targeting Node 16+ (really 12+ with some caveats about async/await and certain package.json exports features). node16/nodenext do not imply that everything uses ESM resolution; they simply correspond to versions of Node where ESM is available side-by-side with CJS. The resolution algorithm switches between CJS (more or less what you get all the time in --moduleResolution node) and Node’s new ESM resolver algorithm based on the input type that Node will see for the given import, which varies based on package.json type, file extension, and import syntax. Even if you are using exclusively CJS source and dependencies in Node 12+, using --moduleResolution node would be incorrect because it doesn’t respect package.json exports, while CJS resolution in Node 12+ does. Eventually, I would like to rename node to something that implies it’s only suitable for legacy code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

5 participants