Skip to content

fix(forms): Value and RawValue should be part of the public API.#45978

Closed
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:tf-value
Closed

fix(forms): Value and RawValue should be part of the public API.#45978
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:tf-value

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented May 12, 2022

Consider a typed group for storing contact information:

declare interface ContactControls {
	name: FormControl<string|null>;
}

contactForm: FormGroup<ContactControls> = ...;

saveForm(form: FormGroup<ContactControls>) {
	service.newContact(contactForm.value);
}

What should be the type of newContact? The answer, of course, is the value type:

declare interface Contact {
	name: string|null;
}

class ContactService {
	newContact(c: Contact) {}
}

This is quite redundant, and therefore, we should allow the value type to be generated automatically. We already have the helper types to do this -- we just need to document and export them. Then, this becomes possible:

class ContactService {
	newContact(c: RawValue<FormGroup<ContactControls>>) {}
}

You can even create an alias, which produces the same type as Contact above:

type Contact = RawValue<FormGroup<ContactControls>>;

This PR was filed in response to a user issue report during 14.0.0-rc.0.

@dylhunn dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: forms cross-cutting: types target: rc This PR is targeted for the next release-candidate forms: strictly typed labels May 12, 2022
@ngbot ngbot bot added this to the Backlog milestone May 12, 2022
@dylhunn dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 12, 2022
@dylhunn dylhunn requested a review from AndrewKushnir May 12, 2022 22:34
@dylhunn dylhunn marked this pull request as ready for review May 12, 2022 22:34
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks good (just a couple minor docs-related comments) 👍

@pullapprove pullapprove bot requested a review from jessicajaniuk May 13, 2022 01:26
jessicajaniuk
jessicajaniuk previously approved these changes May 13, 2022
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from AndrewKushnir May 13, 2022 16:46
AndrewKushnir
AndrewKushnir previously approved these changes May 13, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@givebk-bot

This comment was marked as off-topic.

@antl3x
Copy link

antl3x commented May 14, 2022

@givebk-bot !donate @dylhunn $1

thanks for your PR!

@givebk-bot

This comment was marked as off-topic.

Consider a typed group for storing contact information:

```
declare interface ContactControls {
	name: FormControl<string|null>;
}

contactForm: FormGroup<ContactControls> = ...;

saveForm(form: FormGroup<ContactControls>) {
	service.newContact(contactForm.value);
}
```

What should be the type of `newContact`? The answer, of course, is the value type:

```
declare interface Contact {
	name: string|null;
}

class ContactService {
	newContact(c: Contact) {}
}
```

This is quite redundant, and therefore, we should allow the value type to be generated automatically. We already have the helper types to do this -- we just need to document and export them. Then, this becomes possible:

```
class ContactService {
	newContact(c: RawValue<FormGroup<ContactControls>>) {}
}
```
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub May 16, 2022 16:45
@dylhunn dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 16, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label May 16, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented May 16, 2022

merge-assistance: this PR has the needed approvals (see above), and should be ready to merge.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@jessicajaniuk jessicajaniuk removed the request for review from alxhub May 16, 2022 18:03
@jessicajaniuk jessicajaniuk added action: presubmit The PR is in need of a google3 presubmit and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit labels May 16, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit dba6a60.

jessicajaniuk pushed a commit that referenced this pull request May 16, 2022
)

Consider a typed group for storing contact information:

```
declare interface ContactControls {
	name: FormControl<string|null>;
}

contactForm: FormGroup<ContactControls> = ...;

saveForm(form: FormGroup<ContactControls>) {
	service.newContact(contactForm.value);
}
```

What should be the type of `newContact`? The answer, of course, is the value type:

```
declare interface Contact {
	name: string|null;
}

class ContactService {
	newContact(c: Contact) {}
}
```

This is quite redundant, and therefore, we should allow the value type to be generated automatically. We already have the helper types to do this -- we just need to document and export them. Then, this becomes possible:

```
class ContactService {
	newContact(c: RawValue<FormGroup<ContactControls>>) {}
}
```

PR Close #45978
dylhunn added a commit to dylhunn/angular that referenced this pull request May 17, 2022
…PI."

As per discussion on #fw-forms, this reverts angular#45978 (although the more in-depth comments were kept).
jessicajaniuk pushed a commit that referenced this pull request May 17, 2022
…PI." (#46023)

As per discussion on #fw-forms, this reverts #45978 (although the more in-depth comments were kept).

PR Close #46023
jessicajaniuk pushed a commit that referenced this pull request May 17, 2022
…PI." (#46023)

As per discussion on #fw-forms, this reverts #45978 (although the more in-depth comments were kept).

PR Close #46023
@jessicajaniuk
Copy link
Contributor

@simonmumenthaler We merged it prematurely. It needs some additional design before we can land it and probably won't land until 14.1.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 23, 2022
@dylhunn dylhunn deleted the tf-value branch November 30, 2022 20:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: forms cross-cutting: types forms: strictly typed target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants