Skip to content

Make ModuleResolutionKind.Node10 change backward-compatible - #53139

Merged
Andrew Branch (andrewbranch) merged 2 commits into
microsoft:mainfrom
andrewbranch:ModuleResolutionKind-compat
Mar 7, 2023
Merged

Andrew Branch (andrewbranch) merged 2 commits into
microsoft:mainfrom
andrewbranch:ModuleResolutionKind-compat

Conversation

@andrewbranch

Copy link
Copy Markdown
Member

Fixes #53131

@jakebailey

Copy link
Copy Markdown
Member

That CI result is... weird. is this somehow non-deterministic? Maybe we have to make NodeJs defined as NodeJs = Node10?

@andrewbranch

Copy link
Copy Markdown
Member Author

No, I just forgot to run tests with type checking after I swapped the order.

@andrewbranch

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) cherry-pick to release-5.0

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 7, 2023 •

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the task to cherry-pick this into release-5.0 on this PR at f02c5ea. You can monitor the build here.

@jakebailey

Jake Bailey (jakebailey) commented Mar 7, 2023 •

Copy link
Copy Markdown
Member

Oh, phew.

This reminds me that Debug.formatEnum always picks the first name (so you can deprecate via an assignment, e.g. SyntaxKind.JSDoc and SyntaxKind.JSDocComment), so this PR would actually debug print as the deprecated name (as that's first), but then the enum at runtime is the opposite. Unfortunate.

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Andrew Branch (@andrewbranch), I've opened #53143 for you.

TypeScript Bot (typescript-bot) pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 7, 2023
Component commits:
cdfdfac Make ModuleResolutionKind.Node10 change backward-compatible

f02c5ea Fix order in baseline
@andrewbranch

Copy link
Copy Markdown
Member Author

The order has to be this way to make the enum reverse mapping point to the non-deprecated name, which appears in tsc --traceResolution. Honestly, formatEnum should probably do that too. Deprecated enum members that share a value with a non-deprecated member should always come first if reverse mapping ever matters.

@jakebailey

Copy link
Copy Markdown
Member

Yeah, I agree. Unfortunately in SyntaxKind's case, we must define old names in terms of new names (or vice versa) since the overall values are not stable (compared to this PR where we hardcoded "2").

But since it's vice versa, I can just send a PR that flips them around and it'll work.

@jakebailey

Copy link
Copy Markdown
Member

Although, I think the reason to pick the first one is due to cases like:

enum NodeFlags {
    Number = 1 << 1,
    String = 1 << 2,
    Something = Number | String, // What if String is removed?
}

@andrewbranch

Copy link
Copy Markdown
Member Author

I’m not sure why you can’t forward reference since it gets inlined anyway https://www.typescriptlang.org/play?#code/KYOwrgtgBAolDeAoKKoEEoF4oEYA0yqAQluogL5A

@jakebailey

Copy link
Copy Markdown
Member

If you forward reference, you get an error:

A member initializer in a enum declaration cannot reference members declared after it, including members defined in other enums. ts(2651)

Anyway, not super relevant to this PR besides formatEnum doing the wrong thing, but, I don't really think it's a big deal.

@andrewbranch

Copy link
Copy Markdown
Member Author

Yeah, I meant I’m not sure why we issue that error in the first place.

@jakebailey

Copy link
Copy Markdown
Member

Yeah, though if you add a forward reference, the value it emits is... not right.

Comment thread src/compiler/types.ts
@andrewbranch
Andrew Branch (andrewbranch) merged commit 8458a61 into microsoft:main Mar 7, 2023
@andrewbranch
Andrew Branch (andrewbranch) deleted the ModuleResolutionKind-compat branch March 7, 2023 19:21
Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request Mar 10, 2023
…e-5.0 (#53143)

Co-authored-by: Andrew Branch <andrew@wheream.io>
drivron pushed a commit to scenari/typescript that referenced this pull request Sep 14, 2023
…to release-5.0 (microsoft#53143)

Co-authored-by: Andrew Branch <andrew@wheream.io>
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TS 5.0 - ModuleResolutionKind Enum changed (breaking change that's not listed)

4 participants