Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Document ngModel.$setValidity breaking change in the Changelog #8357

Closed
leefernandes opened this issue Jul 26, 2014 · 15 comments
Closed

Document ngModel.$setValidity breaking change in the Changelog #8357

leefernandes opened this issue Jul 26, 2014 · 15 comments
Assignees
@tbosch
Labels
component: forms frequency: moderate needs: investigation severity: broken expected use type: docs
Milestone
1.3.0-rc.2

Comments

@leefernandes
Copy link
Contributor

At some point in the 1.3.x branch ngModel.$modelValues were set as undefined when invalid. I couldn't find this change documented in the changelog, but it did break validation in our case.

1.2.x behavior: http://codepen.io/ItsLeeOwen/pen/EfwKC
1.3.x behavior: http://codepen.io/ItsLeeOwen/pen/CwALx
@petebacondarwin
Copy link
Contributor

Yeah I think you are right and to be honest if we are decoupling validation from parsing the validators should not be modifying the value but there is much more to it than that.

@petebacondarwin petebacondarwin added this to the 1.3.0-beta.18 milestone Jul 27, 2014
@petebacondarwin petebacondarwin mentioned this issue Jul 27, 2014
Closed
@Ni0rd
Copy link

Ni0rd commented Aug 2, 2014

It seems the breaking change happened with the v1.3.0-beta.12.

@ilplotkin
Copy link

+1 to not modify the value
We had to introduce some hacks to migrate to Angular 1.3 due to this issue.

@btford btford removed the gh: issue label Aug 20, 2014
@btford btford modified the milestones: 1.3.0-beta.19, 1.3.0-beta.20 Aug 22, 2014
@IgorMinar
Copy link
Contributor

@matsko are you on this?

@matsko matsko modified the milestones: 1.3.0-rc.0, 1.3.0-rc.1 Aug 30, 2014
@yiziz
Copy link

yiziz commented Sep 3, 2014

May be this is the logic that's causing trouble. #8080 (comment).

@Narretz
Copy link
Contributor

Narretz commented Sep 5, 2014

Just to clarify, this is not a breaking change in $setValidity:
Previously, the $parsers would validate the viewValue and set the $modelValue to undefined as soon as one of them failed. When you used $setValidity(false), the validity of the object changed out of the validation process.
Next time you changed the input, the $parsers would validate as usual and not catch the new $error, and the $modelValue would be assigned.
After the refactoring, $parsers worked the same, but the validation pipeline checks every $error (set manually or via $validators), and subsequently set $modelValue to undefined.

So it's working more consistently now than before, but I don't think $modelValue should be set to undefined when validation fails. That's what $valid is for.

Narretz referenced this issue in tbosch/angular.js Sep 5, 2014
@tbosch
refactor(ngModelController,formController): centralize and simplify l…
cdbab90 
…ogic

The previous logic for async validation in 
`ngModelController` and `formController` was not maintainable:
- control logic is in multiple parts, e.g. `ctrl.$setValidity`
  waits for end of promises and continuous the control flow
  for async validation
- logic for updating the flags `ctrl.$error`, `ctrl.$pending`, `ctrl.$valid`
  is super complicated, especially in `formController`

This refactoring makes the following changes:
- simplify async validation: centralize control logic
  into one method in `ngModelController`:
  * remove counters `invalidCount` and `pendingCount`
  * use a flag `currentValidationRunId` to separate
    async validator runs from each other
  * use `$q.all` to determine when all async validators are done
- centralize way how `ctrl.$modelValue` and `ctrl.$invalidModelValue` 
  is updated
- simplify `ngModelController/formCtrl.$setValidity` and merge
  `$$setPending/$$clearControlValidity/$$clearValidity/$$clearPending`
  into one method, that is used by `ngModelController` AND
  `formController`
  * remove diff calculation, always calculate the correct state anew,
    only cache the css classes that have been set to not
    trigger too many css animations.
  * remove fields from `ctrl.$error` that are valid and add `ctrl.$success`:
    allows to correctly separate states for valid, invalid, skipped and pending,
    especially transitively across parent forms.
- fix bug in `ngModelController`:
  * only read out `input.validity.badInput`, but not
    `input.validity.typeMismatch`,
    to determine parser error: We still want our `email` 
    validator to run event when the model is validated.
- fix bugs in tests that were found as the logic is now consistent between
  `ngModelController` and `formController`

BREAKING CHANGE:
- `ctrl.$error` does no more contain entries for validators that were
  successful. Instead, they are now saved in `ctrl.$success`.
- `ctrl.$setValidity` now differentiates between `true`, `false`,
  `undefined` and `null`, instead of previously only truthy vs falsy.
@tbosch tbosch assigned tbosch and unassigned matsko Sep 5, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 5, 2014

@matsko I will take this on, as I am in the process of refactoring ngModelController and formConrtroller (see #8941).

@jeffbcross jeffbcross mentioned this issue Sep 9, 2014
Closed
@tbosch tbosch closed this as completed in 7235329 Sep 9, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 9, 2014

Talked with @IgorMinar and decided that the current handling is actually correct: Always set the model value to undefined if either a parser or validator fail:

  • it is hard to explain to a user to have a different behavior between parsers and validators.
  • keeping a stale value in the model seems wrong.

@Narretz
Copy link
Contributor

Narretz commented Sep 9, 2014

I don't think you have understood why people complained about this. They
specifically complained about the fact that the model becomes undefined
after you use $setValidity and then change the input! This is completely
contradictory to "it is hard to explain to a user to have a different
behavior between parsers and validators". People want the $modelValue
even if it is invalid. And that is not even relevant. Parsers do
something different: they transform the viewValue. If that fails, the
$modelValue is undefined, that makes sense. But when a parsed
$modelValue is invalid, this doesn't mean it's stale. It's simply
invalid. There is a fundamental difference between the $modelValue being
there and the $modelValue being invalid, and this difference is being
ignored here.
See #9002 for a PR that
removes $$invalidModelValue and always sets $modelValue.

Am 09.09.2014 21:41, schrieb Tobias Bosch:

Talked with @IgorMinar https://github.com/IgorMinar and decided that
the current handling is actually correct: Always set the model value
to |undefined| if either a parser or validator fail:

  • it is hard to explain to a user to have a different behavior
    between parsers and validators.
  • keeping a stale value in the model seems wrong.

—
Reply to this email directly or view it on GitHub
#8357 (comment).

@Narretz Narretz mentioned this issue Sep 9, 2014
Closed
@Narretz
Copy link
Contributor

Narretz commented Sep 9, 2014

@tbosch Another thing is that

  • it is hard to explain to a user to have a different behavior between parsers and validators.

is a weak argument, especially because noone has yet been confused about the different behaviors of parsers and validators except in this specific case where the new behavior differed from what they expected. Another thing is that especially in such a complex directive we should try to minimize side-effects of APIs. When people use $setValidity they will reasonably expect that it doesn't lead to the $modelValue becoming undefined, especially since that doesn't happen immediately, but only after you change the input. That's what people won't understand and complain about. Not changing this codifies confusing API behavior.

@tbosch
Copy link
Contributor

tbosch commented Sep 9, 2014

@Narretz Sorry, yes, you are right that I understood it wrong what was the breaking change here.

Ok, to not make it a breaking change, we would have to ignore errors for which there is not validator, right?

@tbosch tbosch modified the milestones: 1.3.0-rc.2, 1.3.0-rc.1 Sep 9, 2014
@tbosch tbosch reopened this Sep 9, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 9, 2014

@Narretz Would you like to take a step on creating a docs change for this?

@tbosch
Copy link
Contributor

tbosch commented Sep 9, 2014

Mmh, rethinking this a little bit: Right now, calling ctrl.$setValidity is really not usable without ngModelOptions.allowInvalid. But maybe someone would still like to get only valid values into their scope, so not using the ngModelOptions.allowInvalid.

How about this: whenever the validation runs, we clear all previous except for the newly detected ones. Then we would not have a breaking change in the sense of this issue.

mgallag pushed a commit to mgallag/angular.js that referenced this issue Sep 10, 2014
@tbosch
docs(ngModel): update breaking change from 1.3.0-beta.11 to 1.3.0-bet…
9bc7731 
…a.12

Closes angular#8357
@Narretz
Copy link
Contributor

Narretz commented Sep 10, 2014

Hm, I don't think we have to make it so complicated. I also think it's okay to have a breaking change here, as long as the use case is supported, which is basically: have a property on the ngModelController that always reflects the return value of parsers. That is basically $$invalidModelValue (that's why imo it didn't make much sense to remove it without setting $modelValue to parser result)
I can see how it makes sense that input === viewValue, and model === modelValue, so what if we just had $parsedModelValue? This is for people that don't want to use allowInvalid. For example, you submit a form with invalid values, and the submit handler wants to do own checks on the $parsedModelValue. Or if someone $watches the modelValue and is not interested in validity.

tbosch added a commit to tbosch/angular.js that referenced this issue Sep 10, 2014
@tbosch
fix(ngModel): don’t clear the model when an external validator failed
d20d0e7 
Calling `ctrl.$setValidity()` with a an error key that 
does not belong to a validator in `ctrl.$validator` should
not result in setting the model to `undefined` on the next
input change. This bug was introduced in 1.3.0-beta.12.

Closes angular#8357
Fixes angular#8080
@tbosch tbosch mentioned this issue Sep 10, 2014
Closed
@tbosch
Copy link
Contributor

tbosch commented Sep 10, 2014

How about this:
We restore the old handling, i.e. if we have an error that was set by an external validator we don't clear the model, but if a validator or parser that was registered via $parsers / $validators fails we do.

Via that external validators that call $setValidity can still work, but only get access to the model value when all the normal ones (those registered in $parsers and $validators) are valid. This would be a similar pipeline as we have with the $asyncValidators: the don't get executed unless are sync validators succeed.

See the PR #9016 which is actually a very simple change.

@tbosch tbosch closed this as completed in 9314719 Sep 10, 2014
mgallag pushed a commit to mgallag/angular.js that referenced this issue Sep 10, 2014
@tbosch
docs(ngModel): update breaking change from 1.3.0-beta.11 to 1.3.0-bet…
210bf04 
…a.12

Closes angular#8357
mgallag pushed a commit to mgallag/angular.js that referenced this issue Sep 10, 2014
@tbosch
docs(ngModel): update breaking change from 1.3.0-beta.11 to 1.3.0-bet…
e8e2710 
…a.12

Closes angular#8357
@johnjesse johnjesse mentioned this issue Jan 9, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@tbosch tbosch

Labels
component: forms frequency: moderate needs: investigation severity: broken expected use type: docs
Projects
None yet
Milestone
1.3.0-rc.2
Development

Successfully merging a pull request may close this issue.

fix(ngModel): don’t clear the model when an external validator failed tbosch/angular.js
10 participants
@petebacondarwin @matsko @IgorMinar @btford @tbosch @Narretz @yiziz @Ni0rd @leefernandes @ilplotkin