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

Require module/moduleResolution to match when either is node16/nodenext #54567

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Jun 7, 2023

FAQ if you landed here because of errors upgrading to 5.2

I was using module: commonjs with moduleResolution: nodenext because I need to emit CommonJS but I want package.json "exports" support, etc. How do I do that now?

If you’re running your code in Node.js, or writing a library where someone might run it in Node.js, just use module: nodenext (which defaults moduleResolution to nodenext too). It emits CommonJS by default. ESM is only emitted for .mts files and for .ts files whose nearest ancestor package.json contains "type": "module".

I was using module: esnext with moduleResolution: nodenext because I need to emit ESM but I want package.json "exports" support, etc. How do I do that now?

If you’re running your code through a bundler, switch to moduleResolution: bundler and leave your module setting as esnext.

If you’re running your code in Node.js, or writing a library where someone might run it in Node.js, just use module: nodenext, and either write .mts files, or ensure your package.json contains "type": "module". ESM is emitted for .mts files and for .ts files whose nearest ancestor package.json contains "type": "module".

Why did you change this? It was working fine.

If it was working fine, you got lucky. The original PR description below explains in a bit more detail, but if you were using module: commonjs with moduleResolution: nodenext, one of these consequences was likely to happen:

  • Any .mts files emitted would be completely invalid for Node.js
  • If the package.json had "type": "module", all .ts files emitted would be completely invalid for Node.js
  • Module resolution would likely have resolved to the wrong format of dependency types for dual ESM/CJS packages, which may or may not cause disagreements about how default imports work

If you were using module: esnext, one of these was likely to happen:

  • Any .cts files emitted would be completely invalid for Node.js
  • If the package.json didn’t have "type": "module", all .ts files emitted would be completely invalid for Node.js
  • Module resolution would be completely unsafe and incorrect for Node.js. (If you used an eslint rule to enforce having file extensions on your imports, you can remove that now. TS handles that for you when properly configured.)

Original post

Design discussion: #54735

--module node16 and --moduleResolution node16 were designed as a pair, but historically we’ve allowed them to be mixed with other module/moduleResolution modes. Over time, however, it’s become clear that using them not as a pair doesn’t make a lot of sense and is a real footgun. Many people seem to think to emit ES modules in Node, they need --moduleResolution node16 --module esnext, when what they really need is --module node16 with "type": "module" in their package.json. The former (and no "type": "module") has many problems:

  • Type checking will proceed as if we’re going to emit CommonJS, but after checking, we’ll actually emit ES modules. Node will be incapable of consuming these files, because it (like TypeScript) expects them to be CommonJS format.
  • Type checking of default imports of CJS dependencies will be incorrect since esModuleInterop for CJS implements a different interop strategy than Node ESM.
  • Incorrect import module specifiers will be allowed to compile without error, since module resolution will assume that imports will actually be emitted as require calls, which use a different module resolution algorithm in Node.

Essentially, changing module to something other than node16 when moduleResolution is set to node16 can break every assumption made during module resolution and type checking.

Among the breaks, I found one interesting case in cheerio, where they’re intentionally overriding module twice in order to do dual emit. They currently have additional eslint infrastructure set up to help protect them from some of the aforementioned pitfalls, but the latest release of the package hasn’t managed to escape shipping types with resolution errors. I’m going to submit a PR to that one with a safer alternative.

I also found that @tsconfig/docusaurus (and their new official package @docusaurus/tsconfig) were using a combination intended to simulate --moduleResolution bundler, and they’re happy to switch to that in the next release of @docusaurus/tsconfig.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 7, 2023
@andrewbranch
Copy link
Member Author

@typescript-bot test top200

@andrewbranch
Copy link
Member Author

@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 7, 2023

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 9a775a4. You can monitor the build here.

Update: The results are in!

@andrewbranch
Copy link
Member Author

I could have sworn top200 was a thing

@jakebailey
Copy link
Member

It is, but IIRC can't be run via the bot. I think only @DanielRosenwasser knows how to do it.

@andrewbranch
Copy link
Member Author

Daniel showed me one time, but if I have to open Azure Dev Ops, it’s probably not going to happen 😄

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 8, 2023

It's not that bad

But that one doesn't run against PRs. If you click on the link that the bot posted above, you can go into the pipeline you just triggered.

I think the parameters are pretty easy to set up, with the exception that the dumb thing expects a comment ID to update. I got that via the GraphQL API - but I think you can always use gibberish and it will post a new comment but fail to update the old one.

Here's what I'd suggest running with:

Field Value
Old Typscript Repo Url https://github.com/microsoft/TypeScript.git
Old head reference main
User to tag when the results are ready (Required) andrewbranch
PR ID in github (Required) 54567
Typescript-bot comment ID indicating that the run started (Required) IC_kwDOAT9aAc5eRY10
Machine Count 8
TypeScript entrypoint tsc

Anyway you should try it and see how it goes!

@typescript-bot
Copy link
Collaborator

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/54567/merge:

Something interesting changed - please have a look.

Details

apache/superset

35 of 38 projects failed to build with the old tsc and were ignored

docs/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

backstage/backstage

6 of 13 projects failed to build with the old tsc and were ignored

microsite/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

cheeriojs/cheerio

1 of 3 projects failed to build with the old tsc and were ignored

tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

website/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

facebook/docusaurus

33 of 35 projects failed to build with the old tsc and were ignored

examples/classic-typescript/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

lyswhut/lx-music-desktop

1 of 6 projects failed to build with the old tsc and were ignored

src/common/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/lang/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/main/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/renderer-lyric/tsconfig.json

src/renderer/tsconfig.json

marmelab/react-admin

24 of 25 projects failed to build with the old tsc and were ignored

packages/create-react-admin/tsconfig.json

reworkd/AgentGPT

1 of 2 projects failed to build with the old tsc and were ignored

docs/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

@fatcerberus
Copy link

What's with all the projects that "failed to build with the old tsc and were ignored"? 😮

@andrewbranch
Copy link
Member Author

Probably stuff that needs build steps or configuration that couldn’t be inferred by the runner. Keep in mind the only input this gets is a repo... how many times have you cloned somebody else’s project and not gotten a successful build on the first try? 😄

@DanielRosenwasser
Copy link
Member

I just realized from what I said, it's not parameterized by the count/start index. It's currently hard-coded to 100 and you can definitely change that if you want.

@fatcerberus
Copy link

fatcerberus commented Jun 9, 2023

@andrewbranch Ah, thanks, that makes sense. I had assumed top100 was a curated list (as in, “the top 100 repos we know how to build”) so I got worried there was a recent massive breaking change that snuck under the radar or something.

@andrewbranch
Copy link
Member Author

I don’t remember if this is fully done or not but

@typescript-bot test top200

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 20, 2023

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 9a775a4. You can monitor the build here.

Update: The results are in!

@andrewbranch
Copy link
Member Author

@DanielRosenwasser you are the best 🌟

@typescript-bot
Copy link
Collaborator

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/54567/merge:

Something interesting changed - please have a look.

Details

alibaba/ice

44 of 64 projects failed to build with the old tsc and were ignored

packages/shared/tsconfig.json

packages/miniapp-react-dom/tsconfig.json

backstage/backstage

6 of 13 projects failed to build with the old tsc and were ignored

microsite/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

cheeriojs/cheerio

1 of 3 projects failed to build with the old tsc and were ignored

tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

website/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

facebook/docusaurus

33 of 35 projects failed to build with the old tsc and were ignored

packages/create-docusaurus/templates/classic-typescript/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

examples/classic-typescript/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

fingerprintjs/fingerprintjs

1 of 2 projects failed to build with the old tsc and were ignored

tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

fullcalendar/fullcalendar

1 of 3 projects failed to build with the old tsc and were ignored

scripts/config/tsconfig.node.json

scripts/tsconfig.safe.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

learn-anything/learn-anything

1 of 2 projects failed to build with the old tsc and were ignored

seed/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

lyswhut/lx-music-desktop

1 of 6 projects failed to build with the old tsc and were ignored

src/common/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/lang/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/main/tsconfig.json

  • error TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

src/renderer-lyric/tsconfig.json

src/renderer/tsconfig.json

marmelab/react-admin

24 of 25 projects failed to build with the old tsc and were ignored

packages/create-react-admin/tsconfig.json

redwoodjs/redwood

35 of 48 projects failed to build with the old tsc and were ignored

docs/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

reworkd/AgentGPT

1 of 2 projects failed to build with the old tsc and were ignored

docs/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

web3/web3.js

55 of 62 projects failed to build with the old tsc and were ignored

docs/tsconfig.json

  • error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
    • Project Scope

@andrewbranch andrewbranch marked this pull request as ready for review June 21, 2023 23:46
@andrewbranch andrewbranch changed the title Experiment: Require module/moduleResolution to match when either is node16/nodenext Require module/moduleResolution to match when either is node16/nodenext Jun 21, 2023
@andrewbranch andrewbranch added the Breaking Change Would introduce errors in existing code label Jun 21, 2023
@andrewbranch
Copy link
Member Author

andrewbranch commented Jun 22, 2023

Should --moduleResolution node16 now imply --module node16 if that’s the only valid combo?

Edit: probably not now, because that would change everyone’s emit instead of giving a clear error that this configuration is now invalid.

@SebastienGllmt
Copy link

I'm reading the FAQ and it's not clear to me what just means in the following

If you’re running your code in Node.js, or writing a library where someone might run it in Node.js, just use module: nodenext, and either write .mts files, or ensure your package.json contains "type": "module". ESM is emitted for .mts files and for .ts files whose nearest ancestor package.json contains "type": "module".

Does it means I should remove moduleResolution from the tsconfig and let it be whatever the default value is? If it's the case, could the FAQ be update to explicitly clarify that moduleResolution should be removed from your tsconfig in this case or some other behavior that is clearer than just?

@andrewbranch
Copy link
Member Author

Updated.

@b1rdex
Copy link

b1rdex commented Oct 24, 2023

This looks like a very opinionated breaking change. We have module: node16 with moduleResolution: node on TS 5.1 and everything works well (runs on node.js 20). Updating to 5.2 requires setting moduleResolution: node16 that actually breaks the code. I don't see any particular reason for rewriting the code – it's very hard to switch imports to import() by this requirement.

Do you have any advices? I'll provide some examples of the problem code below.

// example 1. polyfills are in the bootstrap file, usages are in the many files in app.
import wretch from 'wretch';
import dayjs from 'dayjs';
wretch.polyfills({
  fetch: require('node-fetch'),
  URLSearchParams: require('url').URLSearchParams,
});

export async function foo() {
	// am I supposed to replace all usages like this to `await import('wretch')` + importing+awaiting polyfills here?
  const result = await wretch('https://example.com'),
}

// example 2
import dayjsUTC from 'dayjs/plugin/utc';
import dayjsTimezone from 'dayjs/plugin/timezone';
dayjs.extend(dayjsUTC);
dayjs.extend(dayjsTimezone);

export function date(string input) {
	// the same question...
	return dayjs(input)
        .tz('Some/TZ')
        .format('DD.MM.YYYY HH:mm');
}

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Oct 24, 2023

@b1rdex If I just take your code as JS and run it, it doesn't work. What transpilation (i.e. changes from TS) are you wanting to have happen here?

(with package.json "type": "module")
D:\Throwaway\test54567>node foo.js
file:///D:/Throwaway/test54567/foo.js:4
  fetch: require('node-fetch'),
         ^

ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and 'D:\Throwaway\test54567\package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///D:/Throwaway/test54567/foo.js:4:10
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:518:24)
    at async loadESM (node:internal/process/esm_loader:102:5)
    at async handleMainPromise (node:internal/modules/run_main:66:12)

Node.js v19.0.0

(with package.json "type": "commonjs")
D:\Throwaway\test54567>node foo.js
(node:15596) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
D:\Throwaway\test54567\foo.js:1
import wretch from 'wretch';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:360:18)
    at wrapSafe (node:internal/modules/cjs/loader:1088:15)
    at Module._compile (node:internal/modules/cjs/loader:1123:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)
    at Module._load (node:internal/modules/cjs/loader:878:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:82:12)
    at node:internal/main/run_main_module:23:47

@b1rdex
Copy link

b1rdex commented Oct 24, 2023 via email

@RyanCavanaugh
Copy link
Member

The default is "commonjs" if you don't specify

@b1rdex
Copy link

b1rdex commented Oct 25, 2023

@RyanCavanaugh

test.ts

import wretch from 'wretch';
wretch.polyfills({
  fetch: require('node-fetch'),
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  URLSearchParams: require('url').URLSearchParams,
});

export async function foo() {
  // am I supposed to replace all usages like this to `await import('wretch')` + importing+awaiting polyfills here?
  const result = await wretch('https://example.com').get().json();
}

compiles to test.js

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.foo = void 0;
const wretch_1 = __importDefault(require("wretch"));
wretch_1.default.polyfills({
    fetch: require('node-fetch'),
    URLSearchParams: require('url').URLSearchParams,
});
async function foo() {
    const result = await (0, wretch_1.default)('https://example.com').get().json();
}
exports.foo = foo;

@b1rdex
Copy link

b1rdex commented Oct 25, 2023

@RyanCavanaugh second example. Sorry, code in examples had some little problems, fixed now.

test.ts

import dayjs from 'dayjs';
import dayjsUTC from 'dayjs/plugin/utc';
import dayjsTimezone from 'dayjs/plugin/timezone';
dayjs.extend(dayjsUTC);
dayjs.extend(dayjsTimezone);

export function date(input: string) {
  // the same question...
  return dayjs(input).tz('Some/TZ').format('DD.MM.YYYY HH:mm');
}

compiles to test.js

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.date = void 0;
const dayjs_1 = __importDefault(require("dayjs"));
const utc_1 = __importDefault(require("dayjs/plugin/utc"));
const timezone_1 = __importDefault(require("dayjs/plugin/timezone"));
dayjs_1.default.extend(utc_1.default);
dayjs_1.default.extend(timezone_1.default);
function date(input) {
    return (0, dayjs_1.default)(input).tz('Some/TZ').format('DD.MM.YYYY HH:mm');
}
exports.date = date;

@RyanCavanaugh
Copy link
Member

@b1rdex so set "module": "CommonJS", "moduleResolution": "Node10". No config errors, and produces that output.

@jacoblee93
Copy link

@andrewbranch since this change, I'm no longer able to use "module": "CommonJS" together with "moduleResolution": "Node16".

Is there a new way to do this?

I need the module to be CommonJS because I'm using CommonJS. I also need moduleResolution to be Node16 because that's needed for the package.json#exports property to work.

Anyone figure out a way around this issue? Would love to not have to lock our TypeScript dep below 5.2.

@andrewbranch
Copy link
Member Author

  • The next comment from me answers the question
  • I updated the PR description with an FAQ at the top that answers the question

@jacoblee93
Copy link

jacoblee93 commented Nov 13, 2023

  • The next comment from me answers the question
  • I updated the PR description with an FAQ at the top that answers the question

I unfortunately fall into the dual-emit ESM + CJS library developer bucket where my nearest ancestor package.json contains "type": "module". Setting config as described unfortunately didn't fix things.

Sounds like best thing to do is try updating our build script with an alternate package.json populated mid build then?

@andrewbranch
Copy link
Member Author

I recommend looking at https://github.com/isaacs/tshy, which is basically a way to automate that

@jacoblee93
Copy link

Thanks!

@alexandersorokin
Copy link

I am developing a math library that should work both in Node and browsers natively (i.e., without bundling). The library is ESM only. It should support Chrome 70 (there is no top-level await feature).

Before TypeScript 5.2 I used module=ES2020 and moduleResolution=Node16 simultaneously. So TypeScript 5.1 ensured that module imports work both in browsers and Node (it requires adding .js extensions to import statements) and it ensured that there are no top-level awaits in code.

With TypeScript 5.2 if I choose to use module=ES2020 and moduleResolution=bundler it allows extensionless imports that do not work in browsers and Node. If I choose to use module=Node16 and moduleResolution=Node16 it allows top-level awaits that do not work in Chrome 70. And it's ugly to use the Node16 module option for browsers because it can output commonjs (at least theoretically).

Is there a good way to require imports with extensions and not allow top-level awaits? So the library can work both in Node and browsers natively (without bundling).

@andrewbranch
Copy link
Member Author

Not in TypeScript currently—maybe a lint rule?

What’s the name of your library / is it open source somewhere? I’d like to collect a list of examples of TS projects that build unbundled ESM for browsers.

@alexandersorokin
Copy link

@andrewbranch thank you for the clarification. For now, I have decided to use Node16 mode for both module and moduleResolution and ban top-level awaits using third-party ways. If browser or esm only output mode is added to TypeScript, it will be helpful to me. So I will be able to use output results in browsers and Node (and in bundlers too, as a consequence).

But the library I mentioned has not been released yet and is internal. So currently, I can't share it as an example.

nfcampos added a commit to langchain-ai/langsmith-sdk that referenced this pull request Apr 29, 2024
- Switch out implementation for generating parallel CJS build
- Follow guidelines from here microsoft/TypeScript#54567
hinthornw pushed a commit to langchain-ai/langsmith-sdk that referenced this pull request Apr 30, 2024
- Switch out implementation for generating parallel CJS build
- Follow guidelines from here
microsoft/TypeScript#54567
Leftium added a commit to Leftium/WSVS-TS that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.