Skip to content

fix: support union types in Get (#1499) - #1517

Open
katariyaVivek wants to merge 2 commits into
sindresorhus:mainfrom
katariyaVivek:fix/get-union-distribution-1499
Open

katariyaVivek wants to merge 2 commits into
sindresorhus:mainfrom
katariyaVivek:fix/get-union-distribution-1499

Conversation

@katariyaVivek

Copy link
Copy Markdown

Problem

When using Get<BaseType, Path> where BaseType is a union of objects, if an intermediate or leaf property in Path is declared on only some union constituents (but not all), Get improperly collapses to unknown rather than distributing across the union and returning ResolvedType | undefined.

For example:

type Test =
  | {mode: 'test1'; foo: {bar: number}}
  | {mode: 'test2'; foo: {bar: boolean; bar_only_in_test2: string}};

// Previously evaluated to `unknown` because `'bar_only_in_test2'` is not in `keyof ({bar: number} | {bar: boolean; bar_only_in_test2: string})`
type Result = Get<Test, 'foo.bar_only_in_test2'>;

This caused a type safety hole: because Get returned unknown, arbitrary values (e.g. 123) were assignable to Result.

Root Cause

In source/get.d.ts, PropertyOf evaluates Key extends keyof BaseType. In TypeScript, keyof (A | B) produces the intersection of keys ((keyof A) & (keyof B)). When BaseType is a union where some members do not declare Key, Key extends keyof BaseType evaluates to false for the union. Additionally, when BaseType distributes over BaseType extends null | undefined, each individual constituent that lacks Key fell through the array/record checks down to the final unknown fallback. Because unknown | T === unknown in TypeScript, returning unknown on the missing constituent absorbed the valid type from the matching constituent, collapsing the entire union to unknown.

Solution

  1. Added KeysOfUnion<ObjectType> to compute the union of keys across all constituents:
    type KeysOfUnion<ObjectType> = ObjectType extends unknown ? keyof WithStringKeys<ObjectType> : never;
  2. In PropertyOf, captured AllKeys = KeysOfUnion<BaseType> as a default type parameter before distributive conditional evaluation.
  3. When Key is not on the specific constituent being evaluated, checked Key extends AllKeys ? undefined : unknown. Because Key is a known property of the union hierarchy, the missing constituent returns undefined, which unions cleanly with the present constituent (string | undefined), while completely unknown properties continue to fall through to unknown.
  4. Kept array, tuple, and array-like indexing preceding the union fallback so tuple bounds checks and negative index handling are completely preserved.

Testing

Added comprehensive test cases in test-d/get.ts:

  • Regression test for issue bug: Get<> ==> unknown #1499: Get<UnionTest, 'foo.bar_only_in_test2', NonStrict> (fails pre-fix as unknown, passes post-fix as string | undefined).
  • Default strict mode test: Get<UnionTest, 'foo.bar_only_in_test2'> -> string | undefined.
  • Shared property across union members: Get<UnionTest, 'foo.bar', NonStrict> -> number | boolean.
  • Non-existent property on union: Get<UnionTest, 'foo.doesNotExist', NonStrict> -> unknown.
  • Deep nested union test: Get<DeepUnion, 'a.b.c'> -> string | undefined.
  • Deep nested union shared property: Get<DeepUnion, 'a.b.shared'> -> boolean.
  • Intermediate constituent divergence: Get<IntermediateDivergence, 'profile.name'> and Get<IntermediateDivergence, 'details.id'> -> string | undefined.

Verification

  • Typecheck: node --max-old-space-size=6144 ./node_modules/typescript/bin/tsc --project tsconfig.json (0 errors across entire repo).
  • Linter: npx xo source/get.d.ts test-d/get.ts (0 errors, 0 warnings).

Impact

Get<BaseType, Path> now correctly resolves properties on union types without collapsing to unknown. Single objects, tuples, arrays, and non-existent properties are completely unaffected.

Compatibility

Backwards compatible. Fixes #1499.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@sindresorhus

Copy link
Copy Markdown
Owner

I don't think we should change the default behavior. Get already distributes over unions. The unknown is intentional: a property omitted from a member's type can still exist at runtime with another type. I verified that this PR allows a string operation on a number, which then throws.

Declaring foo?: never on the member where it must be absent already gives string | undefined, including for the original example. I'd document that explicit absense instead.

The length case still returns unknown, but that's incomplete coverage of the proposed behavior, not a regression. I'd settle the contract before adding more fallback branches.

@katariyaVivek

Copy link
Copy Markdown
Author

Thanks for the insightful review and explanation @sindresorhus — that makes total sense.

I verified the runtime behavior you mentioned: because TypeScript is structurally typed, an object satisfying an omitted-property branch can indeed hold an unexpected value at runtime, so assuming undefined risks runtime type errors (e.g. attempting string methods on a number).

I've reverted the type change to preserve the existing default behavior and updated this PR to:

  1. Document in the Get JSDoc that omitted union properties intentionally return unknown due to structural typing.
  2. Provide an example showing how to declare explicit absence (prop?: never) when Type | undefined is desired.
  3. Added test assertions in test-d/get.ts verifying both the unknown fallback for omitted properties and Type | undefined when ?: never is specified.

Let me know if this documentation and test update looks good to you!

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.

bug: Get<> ==> unknown

2 participants