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(typings): added typings for getRawValue (TypedFormGroup, TypedFormArray) #150

Merged
merged 1 commit into from
Feb 29, 2020

Conversation

ntziolis
Copy link
Contributor

This PR adds typings for getRawValue on TypedFormGroup and TypedFormArray.

We use getRawValue pretty frequently to access disabled form values, hence added it to the typings as well.

Copy link
Contributor

@maxime1992 maxime1992 left a comment

Choose a reason for hiding this comment

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

LGTM. But.. This makes me realize that getValue type maybe not be true in certain case: if some values are disabled 🤔.

Maybe getRawValue should be TValue and getValue Partial 🤷‍♂️?

But it's not something to deal with this PR :)

@maxime1992 maxime1992 merged commit 6a39e4a into cloudnc:master Feb 29, 2020
@maxime1992
Copy link
Contributor

🎉 This PR is included in version 5.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ntziolis
Copy link
Contributor Author

ntziolis commented Mar 1, 2020

Was thinking the same thing (value should be partial) as I was working on getRawValue. It would be more formally correct. But I'm on the fence, see below:

The same reasoning would also apply to ControlInterface, at least when not in a remap scenario where ControlInterface = FormInterface.

In our own use case we are using the output of forms and push them directly into our api (typescript client fully generated incl request types from openapi sepcs). ControlInterface is usually the generated api request type (which we can't alter). This has reduced the complexity significantly and removed all the ugly as APIRequestTypes from our codebase.

You might already see the problem when dataOutput and dataValue would suddenly be of Partial<ControlInterface> as we would have to as ControlInterface before being able to push the value into the api calls.

I understand the above is just one use case, but I think we should honor the user request to get a specific typing out of the form. Hence in this case we should optimize the lib to reduce the need to use as vs 100% being formally correct.

I feel we are getting very close to the reasoning the angular team does not provide typings for forms :)

@maxime1992
Copy link
Contributor

@ntziolis I do understand 100% that using as syntax is definitely not what we want.

BUT, in regards of type safety there's a huge gap between the type and the reality at run time.

Typescript has no way to know that you're using validators on your form to guarantee that the output object will not be partial but rather the expect type (without partial).

I've started a pretty huge thread here: #147 where I've been thinking of solutions to close that gap and:

  • being able to provide as the output a type that is not Partial<A> but simply A
  • have some run time checks which also brings compile time checks for TS to make sure the type is the one expected

I do encourage you to read the thread and share your own opinion on that matter directly there 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants