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

feat(forms): allow markAsPending to emit events #20212

Closed
wants to merge 1 commit into
base: master
from

Conversation

@Toxicable
Contributor

Toxicable commented Nov 6, 2017

continued from #18676
closes #17958

PR Checklist

Please check if your PR fulfills the following requirements:

  • 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

[x] Feature

What is the current behavior?

markAsPending cannot emit events
Issue Number: #17958

What is the new behavior?

markAsPending now emits events

Does this PR introduce a breaking change?

[x] No
@rossgardt

LGTM. Just some small remarks.

@@ -335,9 +335,13 @@ export abstract class AbstractControl {
/**
* Marks the control as `pending`.

This comment has been minimized.

@rossgardt

rossgardt Nov 7, 2017

@Toxicable as you are touching this method, could you please add more description to the method for the API docs? That'd be awesome. Maybe at least add a reference to this.

This comment has been minimized.

@Toxicable

Toxicable Nov 7, 2017

Contributor

I think the current message is clear enough for the dev to find out what is meant by pending
Also this PR is just for allowing markAsPending to emit events which is not something we specifically write about in the api doc as can be seen by the other methods

(this as{status: string}).status = PENDING;
if (opts.emitEvent !== false) {

This comment has been minimized.

@rossgardt

rossgardt Nov 7, 2017

Being curious: is there a reason for not doing opts.emitEvent == true? I'd find that easier to read.

This comment has been minimized.

@Toxicable

Toxicable Nov 7, 2017

Contributor

Since opts.emitEvent is a optional arg a valid value for it is undefined, so in the default case (being undefined) we still want to emit the event.
It's exclusively in the case where it's false that we don't want to emit,

This comment has been minimized.

@rossgardt

rossgardt Nov 7, 2017

Thank you for the explanation, @Toxicable! Makes total sense now.

@kara kara added the type: bug/fix label Nov 30, 2017

@kara kara self-assigned this Nov 30, 2017

@kara

kara approved these changes Feb 6, 2018

LGTM

@kara

This comment has been minimized.

Contributor

kara commented Feb 6, 2018

@Toxicable I think we need a breaking change notification in the commit message, given that the default behavior is changing (even though it's a bug fix). Can you add one?

@kara kara assigned Toxicable and unassigned kara Feb 7, 2018

@kara

This comment has been minimized.

Contributor

kara commented Feb 7, 2018

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Feb 7, 2018

@kara Done
For your convenience the commit is now:

feat(forms): allow markAsPending to emit events

closes #17958

BREAKING CHANGE:
- `AbstractControl#statusChanges` now emits an event of `'PENDING'` when you call `AbstractControl#markAsPending`

@kara kara requested a review from IgorMinar Feb 7, 2018

@kara

This comment has been minimized.

Contributor

kara commented Feb 7, 2018

@IgorMinar Can you do a public-API review?

@kara kara assigned IgorMinar and unassigned Toxicable Feb 7, 2018

@IgorMinar

if we are making an intentional breaking change and we believe it's ok, because the impact is negligible then we should ensure that the commit message has the following information:

  • what as the old behavior
  • what is the new behavior
  • how do we suggest developers migrate their code if they were affected by this change

Additionally we should also mention in the commit message why we believe that this change is not going to cause lots of developer pain.

otherwise lgtm - I'll let Kara ensure that these things get fixed and send the PR for merge.

@@ -349,9 +349,13 @@ export abstract class AbstractControl {
/**
* Marks the control as `pending`.

This comment has been minimized.

@IgorMinar

IgorMinar Feb 7, 2018

Member

please document the flags and especially what the default behavior is.

feat(forms): allow markAsPending to emit events
closes #17958

BREAKING CHANGE:
- `AbstractControl#statusChanges` now emits an event of `'PENDING'` when you call `AbstractControl#markAsPending`
- Previously it did not emit an event when you called `markAsPending`
- To migrate you would need to ensure that if you are filtering or checking events from `statusChanges` that you account for the new event when calling `markAsPending`
@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Feb 21, 2018

@kara Updated the Commit Message and added some detail to the jsdoc, let me know if you need any other changes.

BREAKING CHANGE:
- `AbstractControl#statusChanges` now emits an event of `'PENDING'` when you call `AbstractControl#markAsPending`
- Previously it did not emit an event when you called `markAsPending`
- To migrate you would need to ensure that if you are filtering or checking events from `statusChanges` that you account for the new event when calling `markAsPending`
@buildsize

This comment has been minimized.

buildsize bot commented Feb 21, 2018

This change will decrease the build size from 10.88 KB to 8.04 KB, a decrease of 2.83 KB (35%)

File name Previous Size New Size Change
bundle.min.js 8.04 KB 8.04 KB 2 bytes (0%)
bundle.min.js.brotli 2.84 KB [deleted]

@vicb vicb closed this in e86b64b Feb 22, 2018

@Toxicable Toxicable deleted the Toxicable:forms-markaspending-events branch Feb 22, 2018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

feat(forms): allow markAsPending to emit events (angular#20212)
closes angular#17958

BREAKING CHANGE:
- `AbstractControl#statusChanges` now emits an event of `'PENDING'` when you call `AbstractControl#markAsPending`
- Previously it did not emit an event when you called `markAsPending`
- To migrate you would need to ensure that if you are filtering or checking events from `statusChanges` that you account for the new event when calling `markAsPending`

PR Close angular#20212

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(forms): allow markAsPending to emit events (angular#20212)
closes angular#17958

BREAKING CHANGE:
- `AbstractControl#statusChanges` now emits an event of `'PENDING'` when you call `AbstractControl#markAsPending`
- Previously it did not emit an event when you called `markAsPending`
- To migrate you would need to ensure that if you are filtering or checking events from `statusChanges` that you account for the new event when calling `markAsPending`

PR Close angular#20212
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment