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.

Sign up for GitHub

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

Jump to bottom

Add new type: Mutable<T, K> (AugmentedMutable<T, K>),AugmentedReadonly<T, K>, and maybe AugmentedPartial<T, K> #112

Open
dosentmatter opened this issue Oct 21, 2019 · 3 comments · May be fixed by #127
Open

Add new type: Mutable<T, K> (AugmentedMutable<T, K>),AugmentedReadonly<T, K>, and maybe AugmentedPartial<T, K> #112

dosentmatter opened this issue Oct 21, 2019 · 3 comments · May be fixed by #127
Labels
accepting PRs enhancement new type

Comments

@dosentmatter
Copy link
Contributor

dosentmatter commented Oct 21, 2019 •
edited

Is your feature request related to a real problem or use-case?

Related to #27. I added Mutable<T> in #111.

For optional properties, we have Optional<T, K> and Required<T, K> (AugmentedRequired<T, K> in our codebase).

To be consistent, we should also have AugmentedMutable<T, K> and AugmentedReadonly<T, K>.

Maybe we should even have AugmentedPartial<T, K> (although Optional<T, K> already does the job) for consistency sake.

We should have some consistency between Partial vs Optional. These two keywords mean the same thing for us. However, in the case of Required, we didn't use a new word and just augmented with AugmentedRequired<T, K>.

Describe a solution including usage in code example

Given Mutable<T> in #111:

export type AugmentedMutable<
  T extends object,
  K extends keyof T = keyof T
> = Omit<T, K> & Mutable<Pick<T, K>>;
export type AugmentedReadonly<
  T extends object,
  K extends keyof T = keyof T
> = Omit<T, K> & Readonly<Pick<T, K>>;
// Same as Optional<T, K>
export type AugmentedPartial<
  T extends object,
  K extends keyof T = keyof T
> = Omit<T, K> & Partial<Pick<T, K>>;

Who does this impact? Who is this for?

TypeScript users.
@dosentmatter dosentmatter changed the title Add new type: Mutable<T, K> (AugmentedMutable<T, K>),AugmentedReadonly<T, K>, and maybe AugmentedPartial<T, K> Add new type: Mutable<T, K> (AugmentedMutable<T, K>),AugmentedReadonly<T, K>, and maybe AugmentedPartial<T, K> Oct 21, 2019
@dosentmatter dosentmatter mentioned this issue Oct 21, 2019
Open
@piotrwitek
Copy link
Owner

piotrwitek commented Oct 21, 2019 •
edited

Hey @dosentmatter,
Thanks for the suggestion.
Regarding consistency, I think it is a good idea to make API surface more intuitive by adding missing aliases for OptionalKeys -> PartialKeys, and AugmentedPartial.
Although be aware AugmentedPartial is only internal name not meant to be used in public API. We would use just Partial as public or alternatively some other but still concise name.

@dosentmatter
Copy link
Contributor Author

dosentmatter commented Oct 22, 2019 •
edited

@piotrwitek, thanks for the response. Yeah, I understand that the AugmentedXXX are internal names. For example, AugmentedRequired instead Required so that we don't name mask the built-in TypeScript Required. I am just using Augmented to refer to types that have key selection by K ie. <T, K> since Augmented is easier to read than <T> vs <T, K>.

Also, even for non-built-ins like Mutable<T>, I would still create AugmentedMutable<T, K> and use Mutable<T> as a helper to keep the code easy to read.

So to summarize, shall we do the following:

  1. Add AugmentedMutable<T, K> (after Add Mutable type #111 is merged) exported as Mutable<T, K> in index.ts.
  2. Add AugmentedReadonly<T, K> exported as Readonly<T, K> in index.ts`.
  3. Rename Optional<T, K> to AugmentedPartial<T, K> exported as Partial<T, K> in index.ts`.
  4. Set Optional<T, K> to be an alias of AugmentedPartial<T, K>.
  5. Set DeepOptional<T, K> to be an alias of DeepPartial<T, K>? Mentioned in Add new type: DeepMutable<T> #114. I think we can skip this because the suggested API will be Partial. We don't need to create the DeepOptional<T, K> alias since DeepOptional<T, K> doesn't currently exist. We only need to create aliases to be backward-compatible. But we can add it to be consistent.
  6. Rename OptionalKeys<T> to PartialKeys<T>? Maybe not, since TypeScript naming applies Partial to the whole object but sets each property to "optional". Since we are talking about Keys, Optional seems to be the correct terminology here.
  7. If 6. is being done, set OptionalKeys<T> to be an alias of PartialKeys<T>.

I think I would do 1-4 and skip 5-7 since:
DeepOptional<T, K> alias doesn't need to be created for backward-compatibility.
I think OptionalKeys<T> is more correct than PartialKeys<T>.
What do you think?

@piotrwitek
Copy link
Owner

piotrwitek commented Oct 22, 2019 •
edited

1 - 4) 👍
5) Yes, let's add DeepOptional, it doesn't cost us anything (just creating alias and adding a small mention in type description in docs) but I guess that some ppl would really like that, maybe even I'll be using it because of OptionalKeys disjointed name.
6) I really like your reasoning here, although it will keep them disjointed, it should still remain intuitive. Let's Skip.
7) Skip

The specification is ready, accepting contributions.

dosentmatter added a commit to dosentmatter/utility-types that referenced this issue Oct 23, 2019
@dosentmatter
Add Writable<T> alias to Mutable<T>
aae2a03 
@piotrwitek commented for `Optional` -> `Partial`, it doesn't cost
anything to add an alias and a mention in docs. See:
piotrwitek#112 (comment)

Similarly, the same can be said for `Writable` -> `Mutable`. We already
have `WritableKeys<T>` -> `MutableKeys<T>` alias.
piotrwitek pushed a commit that referenced this issue Oct 23, 2019
@dosentmatter @piotrwitek
Bugfix/documentation typo fixes (#110)
b83fe21 
* Fix `Brand` "back to top" anchor link

`Brand` is not a Flow utility type.

* Capitalize first letter of `@desc`

* Remove trailing whitespace

* Add back line break before flow URLs

* Add `Writable<T>` alias to `Mutable<T>`

@piotrwitek commented for `Optional` -> `Partial`, it doesn't cost
anything to add an alias and a mention in docs. See:
#112 (comment)

Similarly, the same can be said for `Writable` -> `Mutable`. We already
have `WritableKeys<T>` -> `MutableKeys<T>` alias.

* Export `Writable<T>` alias from `src/index.ts`
@wbadam wbadam linked a pull request Dec 9, 2019 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepting PRs enhancement new type
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature/issue 112 wbadam/utility-types
2 participants
@piotrwitek @dosentmatter