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

feat: add Optional type with granular selection by key #82

Merged
merged 7 commits into from
May 7, 2019

Conversation

ShanaMaid
Copy link
Contributor

@ShanaMaid ShanaMaid commented May 7, 2019

Description

Related issues:

  • Resolved #XXX

Checklist

  • I have read CONTRIBUTING.md
  • I have linked all related issues above
  • I have rebased my branch

For bugfixes:

  • I have added at least one unit test to confirm the bug have been fixed
  • I have checked and updated TOC and API Docs when necessary

For new features:

  • I have added entry in TOC and API Docs
  • I have added a short example in API Docs to demonstrate new usage
  • I have added type unit tests with dts-jest

I add PartOptional

image

@piotrwitek
Copy link
Owner

piotrwitek commented May 7, 2019

That's nice, but why not just use more generic Optional<T, K = any> so it would work like that:

type Person = Optional<Example>; // all props are optional

type Person = Optional<Example, 'age' | 'height'>; // only selected keys are optional

@ShanaMaid
Copy link
Contributor Author

@piotrwitek oh, you are right!I did it, please review once ?

* // Expect: { name: string; age?: string; height?: number; }
* type Props = Optional<Props, 'age' | 'height'>;
*/
export type Optional<T extends {}, K = keyof any> = K extends (keyof T)
Copy link
Owner

Choose a reason for hiding this comment

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

Please change {} to object as this is a convention used in this project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Owner

@piotrwitek piotrwitek left a comment

Choose a reason for hiding this comment

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

It looks great! But we still need to add the missing parts:
[ ] I have added a short example in API Docs to demonstrate new usage --> you can copy what you already added in the JSDoc comments
[ ] I have added type unit tests with dts-jest --> in .spec.ts file just copy tests from other function and try to adapt it for new Optional type, the rest will happen automatically on push

@ShanaMaid
Copy link
Contributor Author

@piotrwitek maybe all is ready?

@piotrwitek
Copy link
Owner

@ShanaMaid awesome job, thanks!
Before I can merge I need to investigate because resulting test type is weird, maybe it can be improved

(Pick<Props, "name" | "visible"> & { age?: number | undefined; }) | (Pick<Props, "name" | "age"> & { visible?: boolean | undefined; })

I'll take a look in 1 hour

Copy link
Owner

@piotrwitek piotrwitek left a comment

Choose a reason for hiding this comment

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

Hey @ShanaMaid, I'm merging it now, thanks for the awesome job 🎉

@piotrwitek piotrwitek merged commit 5125de8 into piotrwitek:master May 7, 2019
@piotrwitek piotrwitek added this to the v3.7.0 milestone May 7, 2019
@piotrwitek
Copy link
Owner

@ShanaMaid I have simplified the Optional type, now it has much clearer resulting type:

export type Optional<T extends object, K extends keyof T = keyof T> = 
Omit<T,K> & Partial<Pick<T, K>>;

I'll be releasing in v3.7

@piotrwitek piotrwitek changed the title feat: add PartOptional feat: add Optional type with optional selection by key May 7, 2019
@piotrwitek piotrwitek changed the title feat: add Optional type with optional selection by key feat: add Optional type with granular selection by key May 7, 2019
@ShanaMaid
Copy link
Contributor Author

ok, that's cool!

@felixfbecker
Copy link

Can we get the inverse to, i.e. Required<T, K>?

@piotrwitek
Copy link
Owner

@felixfbecker definitely yes, as this type turned out to be quite useful.
Could you please create a new issue for it so myself or anyone else can pick it up?

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.

3 participants