Skip to content

feat(forms): improve types for AbstractControl#status - #18442

Closed
Toxicable wants to merge 1 commit into
angular:masterfrom
Toxicable:forms-status-typings
Closed

feat(forms): improve types for AbstractControl#status#18442
Toxicable wants to merge 1 commit into
angular:masterfrom
Toxicable:forms-status-typings

Conversation

@Toxicable

@Toxicable Toxicable commented Aug 1, 2017 •
edited
Loading

Copy link
Copy Markdown

PR Checklist

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

PR Type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)

What is the current behavior?

AbstractControl#status is currently typed to string where as it has four distinct cases of it's value

What is the new behavior?

AbstractControl#status now is of type 'VALID' | 'INVALID' | 'PENDING' | 'DISABLED' giving slightly better support for the developer

Does this PR introduce a breaking change?

[x] Yes (maybe)
[ ] No

If someone is comparing AbstractControl#status to a string other than the ones it will be typed to then after this change TS will throw an error at compile time

Duplicate of #16841 - since I couldn't fix circle ci validating old commits messages

@Toxicable Toxicable mentioned this pull request Aug 1, 2017
Closed
3 tasks
Toxicable
Toxicable commented Aug 1, 2017
View reviewed changes
Comment thread packages/forms/src/model.ts Outdated

@Toxicable Toxicable Aug 1, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

TS throws here if we don't do this silly cast.
It appears there is some issue where we can't assign EventEmitter<T> to Observable<T>

@snerks

snerks commented Aug 2, 2017 •
edited
Loading

Copy link
Copy Markdown

Consider refactoring to a type:

export type AbstractControlStatus = 'VALID'|'INVALID'|'PENDING'|'DISABLED';

Swap out inline usage with the newly-defined type.

@Toxicable

Toxicable commented Aug 2, 2017

Copy link
Copy Markdown
Author

The issue with that is it'll be yet another export, weather it's public or not I'm unsure but either way cutting down on exports is always good

@snerks

snerks commented Aug 2, 2017

Copy link
Copy Markdown

OK. So, keep the type but remove the export keyword?

@Toxicable Toxicable mentioned this pull request Aug 23, 2017
Closed
@Toxicable
Toxicable force-pushed the forms-status-typings branch 2 times, most recently from f0502dd to 5619ce8 Compare August 23, 2017 22:31
@kyr0

kyr0 commented Aug 24, 2017

Copy link
Copy Markdown

Merging this would help our team maintaining a clean coding style 馃憤

@Toxicable
Toxicable force-pushed the forms-status-typings branch 2 times, most recently from 79cd803 to 572d1d2 Compare August 24, 2017 20:00
@Toxicable
Toxicable force-pushed the forms-status-typings branch from 572d1d2 to 15b0065 Compare August 24, 2017 21:01
@ngbot

ngbot Bot commented Jan 30, 2018

Copy link
Copy Markdown

Hi @Toxicable! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@kara kara added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 18, 2018
@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019
@pullapprove
pullapprove Bot requested a review from AndrewKushnir May 8, 2020 23:04
@AndrewKushnir

AndrewKushnir commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

Closing this PR in favor of more recent #38406 and #38906. Thank you.

@angular-automatic-lock-bot

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

Copy link
Copy Markdown

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: review The PR is still awaiting reviews from at least one requested reviewer area: forms cla: yes cross-cutting: types feature Label used to distinguish feature request from other issues

Projects

None yet

Milestone

Forms type improvements

Development

Successfully merging this pull request may close these issues.

8 participants

@Toxicable @snerks @kyr0 @AndrewKushnir @vicb @jasonaden @kara @googlebot