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

Make ModuleResolutionKind.Node10 change backward-compatible #53139

Merged

Conversation

andrewbranch
Copy link
Member

Fixes #53131

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 7, 2023
@jakebailey jakebailey added this to the TypeScript 5.0.2 milestone Mar 7, 2023
@jakebailey
Copy link
Member

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

@andrewbranch
Copy link
Member Author

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

@andrewbranch
Copy link
Member Author

@typescript-bot cherry-pick to release-5.0

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 7, 2023

Heya @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
Copy link
Member

jakebailey commented Mar 7, 2023

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
Collaborator

Hey @andrewbranch, I've opened #53143 for you.

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
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
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
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
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
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
Member Author

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

@jakebailey
Copy link
Member

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

@andrewbranch andrewbranch merged commit 8458a61 into microsoft:main Mar 7, 2023
18 checks passed
@andrewbranch andrewbranch deleted the ModuleResolutionKind-compat branch March 7, 2023 19:21
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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