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

Modify DeepPartial to work with 'unknown' type #30

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Modify DeepPartial to work with 'unknown' type #30

merged 1 commit into from
Jul 1, 2020

Conversation

b-houghton
Copy link
Contributor

Fixes #29

Credit for this fix goes to @OliverJAsh, who wrote this to fix the same problem in reduxjs/redux.

Source: Oliver Joseph Ash, reduxjs/redux#3369
files#diff-b52768974e6bc0faccb7d4b75b162c99
Copy link
Contributor

@stevehanson stevehanson left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for this contribution, @b-houghton!

@stevehanson stevehanson merged commit 1bac2a4 into thoughtbot:master Jul 1, 2020
@stevehanson
Copy link
Contributor

@b-houghton I just realized CI didn't run on this PR for some reason (maybe it's not running on forks). After I merged into master, I'm getting a test failure because this no longer compiles:

// factory.test.ts:29
it('accepts partials of nested objects', () => {
  const user = userFactory.build({ address: { city: 'Ann Arbor' } }); // Compile error: Property state is missing but required in type { city: string, state: string }
  expect(user.address).toMatchObject({ city: 'Ann Arbor', state: 'MI' });
});

Basically, DeepPartial is no longer working for the type below and requiring a full address to be passed:

type User = {
  id: string;
  name: string;
  address?: { city: string; state: string };
};

I can take a look later but curious if you have any thoughts. I might have to revert this till we get a fix.

@b-houghton
Copy link
Contributor Author

@stevehanson Please forgive me; I'm not sure why this happened. I did run the tests before committing the change as the guidelines for contributing request. I may not have time to follow up on this this week, but will try to return to it by sometime early next week.

@stevehanson
Copy link
Contributor

@b-houghton no worries! I reverted the PR for now, so there’s no rush. I’ll also try to take a look soon.

@b-houghton
Copy link
Contributor Author

@stevehanson I have studied the TypeScript documentation and think I understand better what is happening. First, the issue I reported occurs because the type of DeepPartial<unknown> is incompatible with unknown. The TypeScript release notes that introduce unknown point out the type of a similar mapped type when the type parameter is unknown:

// Homomorphic mapped type over unknown

type T50<T> = { [P in keyof T]: number };
type T51 = T50<any>;  // { [x: string]: number }
type T52 = T50<unknown>;  // {}

Second, based on the error message that you pointed out, I think that the solution that I submitted in this PR fails because the condition T[P] extends object does not evaluate to true for the optional property address, because its type is {city: string, state: string} | undefined.

I have thought of two possible solutions. The first would recurse except when T[P] evaluates to unknown or any:

export type DeepPartial<T> = { [P in keyof T]?: unknown extends T[P] ? T[P] : DeepPartial<T[P]> };

The other possible solution extends the present one by allowing for optional object properties:

export type DeepPartial<T> = { [P in keyof T]?: T[P] extends object | undefined ? DeepPartial<T[P]> : T[P] };

Based on the TypeScript documentation for distributive conditional types, T[P] extends object | undefined will evaluate to true both for properties that are assignable to type object and properties that that are assignable to type undefined. I don't think allowing the latter would have any negative effect given that the current implementation of DeepPartial recurses for all types, including undefined.

What do you think of these possibilities?

@stevehanson
Copy link
Contributor

@b-houghton this is awesome. Thanks for putting so much thought and research into this, and good find that the error was because the address was typed as object | undefined.

I really like your first solution because it explicitly only changes our current implementation for the unknown case we're trying to solve for.

The second option fixes the case where the object could be undefined, like in our tests, but would it work for object | null or object | string, etc?

@b-houghton
Copy link
Contributor Author

@stevehanson I'm glad you asked. Upon investigation, the answer is no. I had thought, based on the documentation for distributive conditional types, that it would work, but it doesn't. I admittedly don't yet understand why. Regardless, it looks like the first solution I proposed is the only viable one of the two. Shall I open a new PR for it?

@stevehanson
Copy link
Contributor

@b-houghton yeah, that'd be great! I should be able to review it sometime this week. Thanks again!

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.

DeepPartial type cannot handle 'unknown' type
2 participants