Skip to content

feat(forms): expose form status values to public API#27665

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

feat(forms): expose form status values to public API#27665
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Dec 14, 2018 •
edited by ghost
Loading

Change the public API to include the form status values allowing status comparisons to the existing status constants instead of redefining within applications.

Closes #27389

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The form status constants are not exposed to the public API. This causes remaking constants/string when comparing form statuses.

Issue Number: #27389

What is the new behavior?

The public API exposes these values from the model, allowing them to be imported/used.

Does this PR introduce a breaking change?

  • Yes
  • No

@ghost
Copy link
Author

ghost commented Dec 14, 2018

May I please have some guidance for adding exports to the public API? :)

kara
kara suggested changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

We also will need some tests.

packages/forms/src/model.ts Outdated
Copy link
Contributor

@kara kara Jan 10, 2019

Choose a reason for hiding this comment

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

I think we should avoid exporting something as generic as VALID or DISABLED. Can we instead add static fields to AbstractControl for these? Like AbstractControl.VALID?

Copy link
Author

@ghost ghost Jan 11, 2019

Choose a reason for hiding this comment

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

Yep, on it (and the tests)!

@kara kara 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 target: major This PR is targeted for the next major release labels Jan 10, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 10, 2019
@kara kara assigned ghost Jan 10, 2019
@kara
Copy link
Contributor

kara commented Jan 10, 2019

@ChrisDahmCS Re public API guard, you'll need to run the bazel test

yarn bazel test tools/public_api_guard/...

@ghost
Copy link
Author

ghost commented Jan 11, 2019

@kara
I apologize but I am still a bit unfamiliar with this part of the process. If there is a change that causes the Bazel test to fail, should I be accepting/committing the new "golden file" with the PR as well?

@kara
Copy link
Contributor

kara commented Jan 18, 2019

Yes, if the change in the golden file is expected. If we are adding a new public API (like in this case), we would expect that to change.

@ghost ghost requested review from a team January 23, 2019 00:28
@JoostK
Copy link
Member

JoostK commented Jan 23, 2019

Why not a const enum instead, that tree shakes much better.

feat(forms): add static form status fields
11e7786 
Add static form status fields to the abstract control. Now comparisons to the status can use these static fields.

Closes #27389
@ghost
Copy link
Author

ghost commented Jan 23, 2019

So I feel I should stop to ask more questions:
As of right now I need to change the static names back to uppercase if those will be kept. From the comment by @JoostK as well as others on issues like #27389, would it be preferred for me to add or go the route of adding a type/enum?

@kara
Copy link
Contributor

kara commented Jan 24, 2019

@ChrisDahmCS I'm good with a const enum too. @IgorMinar Do you have an opinion?

@jorroll jorroll mentioned this pull request Aug 2, 2019
Open
@AndrewKushnir AndrewKushnir added the forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. label May 8, 2020
@pullapprove pullapprove bot requested a review from AndrewKushnir May 8, 2020 23:49
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Dec 8, 2020

Hi @ChrisDahmCS, thanks for creating this PR. This change seems to be related (and actually blocked by) the work on the strict form types. Strict form types improvements are on the Roadmap, but there is no ETA at this moment. I'm going to close this PR for now and we'll get back to the possibility of exposing statuses as public APIs once strict form type improvements are implemented. Thank you.

@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jan 8, 2021

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 Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: forms cla: yes forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

Expose form status types (VALID, INVALID, PENDING, DISABLED)

4 participants

@kara @JoostK @AndrewKushnir @googlebot