Skip to content

Get: Distribute over unions so a key missing on some members yields undefined#1463

Open
ardittirana wants to merge 2 commits into
sindresorhus:mainfrom
ardittirana:fix-get-union-distribution
Open

Get: Distribute over unions so a key missing on some members yields undefined#1463
ardittirana wants to merge 2 commits into
sindresorhus:mainfrom
ardittirana:fix-get-union-distribution

Conversation

@ardittirana

Copy link
Copy Markdown

Fixes #1205

What was wrong

For a union base type, Get accessed the key on each member, but a member that lacked the key resolved to unknown — and unknown absorbs the rest of the union. So:

Get<{a: number} | {b: string}, 'a'> // was `unknown`, expected `number | undefined`
Get<{data: {type: 'x'; v: number} | {type: 'y'}}, 'data.v'> // was `unknown`

Fix

PropertyOf now detects a union first and, when distributing over its members, treats a missing key as undefined for that member. A missing key on a non-union type still resolves to unknown (the existing, documented behaviour — preserved by an explicit MissingProperty parameter).

Tests

Added cases to test-d/get.ts for: key present on all members, key present on some members (→ number | string | undefined), a direct union base type, and the unchanged non-union unknown case. npm run test:tsd and xo pass; no existing assertions changed.

… `undefined`

For a union base type, `Get` accessed the key on each member but a member
lacking the key resolved to `unknown`, which absorbed the whole union (e.g.
`Get<{a: number} | {b: string}, 'a'>` was `unknown` instead of `number | undefined`).

`PropertyOf` now checks for a union first and, when distributing, treats a
missing key as `undefined` for that member, while a missing key on a non-union
type still resolves to `unknown` (unchanged, documented behaviour).

Fixes sindresorhus#1205.

@som-sm som-sm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A missing key on a non-union type still resolves to unknown (the existing, documented behaviour — preserved by an explicit MissingProperty parameter).

I think the existing behaviour here is incorrect, this should also be undefined. More so, if for unions it's going to be undefined. Let's make it consistently undefined for missing properties.

Following review: a key that is not present on the base type now resolves
to `undefined` rather than `unknown`, for both union and non-union base
types. This matches the behaviour of deep-key libraries like lodash, and
makes the result consistent with the union-distribution case (where a key
missing on some members already became `undefined`).

The `MissingProperty` parameter on `SinglePropertyOf` is no longer needed
and is removed. Tests for out-of-bounds and missing-key access are updated
from `unknown` to `undefined`.
@ardittirana

Copy link
Copy Markdown
Author

Makes sense — consistency wins here, and you're right that it's odd to special-case non-unions once unions resolve missing keys to undefined. I've updated it so a missing key resolves to undefined for both union and non-union base types, matching lodash/dot-prop runtime behaviour.

Changes:

  • PropertyOf no longer distinguishes the two cases; the now-redundant MissingProperty parameter on SinglePropertyOf is removed.
  • The doc note now reads "Returns undefined if Key is not a property of BaseType".
  • Updated the out-of-bounds / missing-key tests (tuples, array index, Get<{a: number}, 'b'>, etc.) from unknown to undefined.

Full tsd suite passes and lint is clean. Let me know if you'd prefer any wording tweaks in the docs.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible incorrect behavior of 'GET<>'

2 participants