Port PR 61805 - module=node20

[jakebailey]

Ports microsoft/TypeScript#61805

[Copilot]

Pull Request Overview

This PR ports TypeScript PR 61805 to address changes related to the module=node20 configuration, focusing on updates to module resolution behavior and module format handling in Node.js 20 environments.

Key changes include:

• Removal of legacy module resolution validation errors
• Updated module output formats for ESM vs CJS contexts
• Improved import assertion to import attributes error messaging
• Corrected module resolution strategy selection

Reviewed Changes

Copilot reviewed 300 out of 645 changed files in this pull request and generated no comments.

File	Description
Multiple baseline error files	Removed TS5109 errors requiring Node16 module resolution with Node20 module setting
Multiple baseline JavaScript output files	Updated module compilation output to use proper ESM syntax instead of CommonJS when appropriate
Import assertion baseline files	Changed error messages from import assertions to import attributes deprecation warnings
Module resolution trace files	Updated to show Node16 resolution strategy instead of Bundler

[jakebailey]

Hm, data race...

[jakebailey]

I can't figure out how to deal with the circularity errors when compiling concurrently. These are the kinds of errors that we have been silencing by skipping the tests (concurrentSkippedErrorBaselines), which would defeat the purpose of these checks. I am not sure if this means the tests are "bad" themselves for introducing those circularities, or if there's a bug in my (or someone else's) ported code that's causing the issue.

[jakebailey]

The symbol baselines doing something like:

Implies that we are somehow choosing the wrong import when compiling concurrently, so I hope it's just that kind of bug, but the race detector does not detect anything.

[andrewbranch]

The circularities are real. I believe what’s going on here is this: suppose you have a big circle of imports: a.ts → b.ts → c.ts → d.ts → e.ts → a.ts. In single-threaded mode, we always start with a.ts because of file order / source order, and we go off down the chain:

• resolve import of b.ts
  • resolve import of c.ts
    • resolve import of d.ts
      • resolve import of e.ts
        • resolve import of a.ts - already in progress! set import of a.ts to any
        • return any
      • return any
    • return any
  • return any
• return any
• import of b.ts got set to any while resolving import of c.ts, so a circularity occurred
• report an error on the import of b.ts

With this algorithm, the circle can be arbitrarily long, but we only ever report an error on the place where we start, which is semi-arbitrary but deterministic by file order, source order, etc. But if we divvy up all the files between multiple independent checkers, there’s a strong chance multiple of them will start resolving the chain in different places, resulting in the same error being reported in a different file.

I’m not sure what we want to actually do about this.

• It would be easy to issue the circularity error on every file instead of an arbitrary point, but that would probably be annoying
• Otherwise, we would have to save the resolution stack in order to apply some arbitrary but deterministic way of picking where to issue the error regardless of where we started. (We don’t currently maintain that stack at all; we just notice a mutation happened and know a circularity occurred, but we have no breadcrumbs for it. It shouldn’t be hard to add, though.)

[jakebailey]

That's what I thought, unfortunately.

As a non-fix, should the tests be fixed to not use a circular reference? It seems like those which break aren't intentionally testing that behavior.

[andrewbranch]

Yeah, I think that would be fine for now.

[jakebailey]

microsoft/TypeScript#62568 does that, and it indeed makes this PR pass tests when applied to the submodule.

[jakebailey]

Curious difference here; I am not sure if this is something I did? Must have?

[jakebailey]

The code that reports this looks a bit different than Strada, so probably not worth blocking this PR on.