Skip to content

fix(common): preserve literal key union in KeyValuePipe.transform() - #70200

Open
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/common-keyvalue-pipe-literal-key-widening
Open

fix(common): preserve literal key union in KeyValuePipe.transform()#70200
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/common-keyvalue-pipe-literal-key-widening

Conversation

@arturovt

Copy link
Copy Markdown
Contributor

Previously, when you passed an object typed like
Record<'a' | 'b', number> into the keyvalue pipe, TypeScript would "forget" that the keys could only ever be 'a' or 'b', and just tell you the key was a plain string instead. So code like this used to fail to compile, even though it's correct:

  const input: Record<'a' | 'b', number> = {a: 1, b: 2};
  const result = pipe.transform(input);
  const key: 'a' | 'b' = result[0].key; // error: string is not 'a' | 'b'

This happened because the pipe has multiple overloaded versions of transform(), and TypeScript checks them top to bottom, using the first one that matches. The "number keys" overload was listed first, and it happened to also match string-keyed objects by accident, so it "won" before the correct "string keys" overload ever got a chance to run.

The fix just reorders those two overloads so the string-keys one is checked first. Nothing about runtime behavior changes — objects with actual numeric keys (e.g. Record<1 | 2, string>) still correctly report their keys as plain string, matching what Object.keys() really returns at runtime.

@pullapprove
pullapprove Bot requested a review from atscott August 13, 2026 21:49
@angular-robot angular-robot Bot added the area: common Issues related to APIs in the @angular/common package label Aug 13, 2026
@ngbot ngbot Bot added this to the Backlog milestone Aug 13, 2026
@JeanMeche JeanMeche added the action: global presubmit The PR is in need of a google3 global presubmit label Aug 13, 2026
@JeanMeche

Copy link
Copy Markdown
Member

I'll run a TGP to see how breaking that is.

@JeanMeche

Copy link
Copy Markdown
Member

We have a passing TGP.

Can you please update the goldens.

@JeanMeche JeanMeche added target: minor This PR is targeted for the next minor release and removed target: minor This PR is targeted for the next minor release labels Aug 14, 2026
@JeanMeche
JeanMeche removed the request for review from atscott August 14, 2026 16:33
@JeanMeche JeanMeche added the target: patch This PR is targeted for the next patch release label Aug 14, 2026
Previously, when you passed an object typed like
Record<'a' | 'b', number> into the `keyvalue` pipe, TypeScript would
"forget" that the keys could only ever be 'a' or 'b', and just tell
you the key was a plain `string` instead. So code like this used to
fail to compile, even though it's correct:

```ts
  const input: Record<'a' | 'b', number> = {a: 1, b: 2};
  const result = pipe.transform(input);
  const key: 'a' | 'b' = result[0].key; // error: string is not 'a' | 'b'
```

This happened because the pipe has multiple overloaded versions of
transform(), and TypeScript checks them top to bottom, using the
first one that matches. The "number keys" overload was listed first,
and it happened to also match string-keyed objects by accident, so
it "won" before the correct "string keys" overload ever got a
chance to run.

The fix just reorders those two overloads so the string-keys one is
checked first. Nothing about runtime behavior changes — objects with
actual numeric keys (e.g. Record<1 | 2, string>) still correctly
report their keys as plain `string`, matching what Object.keys()
really returns at runtime.
@arturovt
arturovt force-pushed the fix/common-keyvalue-pipe-literal-key-widening branch from 6b54443 to 0fb2453 Compare August 14, 2026 19:57
@pullapprove
pullapprove Bot requested review from JeanMeche and atscott August 14, 2026 19:57
@arturovt

Copy link
Copy Markdown
Contributor Author

@JeanMeche updated goldens (public API), no other changes have been made.

@JeanMeche JeanMeche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

@JeanMeche
JeanMeche removed the request for review from atscott August 14, 2026 19:59
@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 14, 2026
@JeanMeche

Copy link
Copy Markdown
Member

caretaker note: Presubmit failure is pre-exisiting. This is good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: global presubmit The PR is in need of a google3 global presubmit action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants