Require module/moduleResolution to match when either is node16/nodenext #54567
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test top200 |
|
@typescript-bot test top100 |
|
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! |
|
I could have sworn top200 was a thing |
|
It is, but IIRC can't be run via the bot. I think only @DanielRosenwasser knows how to do it. |
|
Daniel showed me one time, but if I have to open Azure Dev Ops, it’s probably not going to happen 😄 |
|
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:
Anyway you should try it and see how it goes! |
|
@andrewbranch Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
What's with all the projects that "failed to build with the old tsc and were ignored"? 😮 |
|
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? 😄 |
|
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. |
|
@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. |
|
I don’t remember if this is fully done or not but @typescript-bot test top200 |
|
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! |
|
@DanielRosenwasser you are the best 🌟 |
|
@andrewbranch Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
andrewbranch
changed the title
|
Should Edit: probably not now, because that would change everyone’s emit instead of giving a clear error that this configuration is now invalid. |
|
I'm reading the FAQ and it's not clear to me what
Does it means I should remove |
|
Updated. |
|
This looks like a very opinionated breaking change. We have 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');
} |
|
@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? |
|
Well, that’s strange. The only guess I have - we don’t specify “type” in package.json at all. I can create minimal reproduction if you want to see the build
|
|
The default is |
|
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; |
|
@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; |
|
@b1rdex so set |
Anyone figure out a way around this issue? Would love to not have to lock our TypeScript dep below 5.2. |
|
I unfortunately fall into the dual-emit ESM + CJS library developer bucket where my nearest ancestor package.json contains Sounds like best thing to do is try updating our build script with an alternate |
|
I recommend looking at https://github.com/isaacs/tshy, which is basically a way to automate that |
|
Thanks! |
|
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 With TypeScript 5.2 if I choose to use 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). |
|
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. |
|
@andrewbranch thank you for the clarification. For now, I have decided to use But the library I mentioned has not been released yet and is internal. So currently, I can't share it as an example. |
- Switch out implementation for generating parallel CJS build - Follow guidelines from here microsoft/TypeScript#54567
- Switch out implementation for generating parallel CJS build - Follow guidelines from here microsoft/TypeScript#54567
FAQ if you landed here because of errors upgrading to 5.2
I was using
module: commonjswithmoduleResolution: nodenextbecause 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 defaultsmoduleResolutiontonodenexttoo). It emits CommonJS by default. ESM is only emitted for.mtsfiles and for.tsfiles whose nearest ancestor package.json contains"type": "module".I was using
module: esnextwithmoduleResolution: nodenextbecause 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: bundlerand leave yourmodulesetting asesnext.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.mtsfiles, or ensure your package.json contains"type": "module". ESM is emitted for.mtsfiles and for.tsfiles 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: commonjswithmoduleResolution: nodenext, one of these consequences was likely to happen:.mtsfiles emitted would be completely invalid for Node.js"type": "module", all.tsfiles emitted would be completely invalid for Node.jsIf you were using
module: esnext, one of these was likely to happen:.ctsfiles emitted would be completely invalid for Node.js"type": "module", all.tsfiles emitted would be completely invalid for Node.jsOriginal post
Design discussion: #54735
--module node16and--moduleResolution node16were designed as a pair, but historically we’ve allowed them to be mixed with othermodule/moduleResolutionmodes. 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 node16with"type": "module"in their package.json. The former (and no"type": "module") has many problems:esModuleInteropfor CJS implements a different interop strategy than Node ESM.requirecalls, which use a different module resolution algorithm in Node.Essentially, changing
moduleto something other thannode16whenmoduleResolutionis set tonode16can 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
moduletwice 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.