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

fix: handle 'any' values properly in 'ControlOf' #60

Merged
merged 1 commit into from
Nov 22, 2020
Merged

fix: handle 'any' values properly in 'ControlOf' #60

merged 1 commit into from
Nov 22, 2020

Conversation

rafaelss95
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md#commit
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The ControlsOf type recognizes any values as FormArrays.

Captura de Tela 2020-11-19 às 12 02 38

Issue Number: Related to #59.

What is the new behavior?

The ControlsOf type recognizes any values as any.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@@ -133,7 +132,9 @@ export type AbstractControlsOf<T extends Obj> = {
* ])
* });
* */
export type ControlOf<T> = [T] extends [any[]]
export type ControlOf<T> = unknown extends T
? T
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
? T
? AbstractControl<T>

@@ -133,7 +132,9 @@ export type AbstractControlsOf<T extends Obj> = {
* ])
* });
* */
export type ControlOf<T> = [T] extends [any[]]
export type ControlOf<T> = unknown extends T
Copy link
Contributor

@itayod itayod Nov 22, 2020

Choose a reason for hiding this comment

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

I didn't like you used unknown to detect any type, let's use a safer way.
please add and use this utility:

type ExtractAny<T> = T extends Extract<T, string & number & boolean & object & null & undefined> ? any : never;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itayod Excuse me, I'm not sure I got this suggestion 🤔. Could you explain how it would work as expected with a full example? I was unable to make it work properly.

Btw, is there any reason of disliking the use of unknown? Do you have any cases where it would break? Please let me know, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rafaelss95 I actually tried it in my project and it broke my code. About the example there you go:

type ExtractAny<T> = T extends Extract<T, string & number & boolean & object & null & undefined> ? any : never;

export declare type ControlOf<T> = [T] extends ExtractAny<T> ? AbstractControl<T> : [T] extends [Array<any>] ?
  FormArray<ControlOf<UnwrapArray<T>>>
  : [T] extends [object] ? FormGroup<ControlsOf<T>>
  : FormControl<T>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, you wrapped T... now I was able to make it work :)

Copy link
Contributor Author

@rafaelss95 rafaelss95 Nov 22, 2020

Choose a reason for hiding this comment

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

I noticed another strange behavior (with both solutions):

Captura de Tela 2020-11-22 às 11 54 33
Captura de Tela 2020-11-22 às 11 54 45

Note that when you have a ReadonlyArray, it infers it as a FormGroup, while a FormArray would be expected.


Edit: just found the bug.

type ArrayType<T> = T extends Array<infer R> ? R : any;

To handle ReadonlyArray properly it should be:

 type ArrayType<T> = T extends ReadonlyArray<infer R> ? R : any; 

As it would be a bit out of the context of this PR, I would prefer to open another PR with this and another fixes about acceptance of ReadonlyArray.

Copy link
Contributor

@itayod itayod left a comment

Choose a reason for hiding this comment

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

LGTM

@itayod itayod merged commit 779e083 into ngneat:master Nov 22, 2020
@rafaelss95 rafaelss95 deleted the fix/controls-of-typing branch November 22, 2020 15:21
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.

None yet

2 participants