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

feat(ngModelOptions): add allowInvalid option #8313

Closed
wants to merge 1 commit into from
Closed

feat(ngModelOptions): add allowInvalid option #8313

wants to merge 1 commit into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Jul 24, 2014

This option allows to write invalid values to the model instead of having them become undefined.

Closes #8290

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8313)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@shahata
Copy link
Contributor Author

shahata commented Jul 24, 2014

The main idea here is that $parsers no longer change the value to undefined in case the value is invalid. Instead, we let $$runValidators do that in turn - and this is a refactoring we can do without breaking a single test. From there, adding the option to allow writing invalid values to the model is trivial.

This is still a work in progress since I haven't added any docs yet, but I wanted to put it here to get some feedback before I continue.

@shahata shahata added cla: yes and removed cla: no labels Jul 24, 2014
@Narretz
Copy link
Contributor

Narretz commented Jul 24, 2014

Hey @shahata, thanks for working on this. I think we should go one step further and completely decouple $parsers and $validators. With $validators, $parsers should now always return a representation of the model. The problem with date and number is obviously that we have to have some sort of validation to be able to parse them, but that also means the $validators can cleanly expect a Date object respectively a number.
I've also seen that @matsko has expanded his PR for async validation (https://github.com/angular/angular.js/pull/8267/files) and the validation API has changed again. We'll have to wait how this one plays out.

@btford btford added this to the Backlog milestone Jul 24, 2014
@jgrenat
Copy link

jgrenat commented Aug 11, 2014

Hey! Is there any news about this PR?

@shahata
Copy link
Contributor Author

shahata commented Aug 12, 2014

Waiting for #8267

@shahata
Copy link
Contributor Author

shahata commented Aug 30, 2014

I've rebased this since #8267 was merged

@matsko matsko self-assigned this Aug 30, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0-rc.1, Backlog Aug 30, 2014
@IgorMinar
Copy link
Contributor

I'm fine with this making it into rc.1

cc: @tbosch

expect(inputElm).toBeInvalid();
});

it('should assign invalid values from $parsers if allowInvalid is true', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this test. If the parser decides that the value is not parseable, we still don't get a value in the model, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. This is a relic from when we still had validators inside $parsers pipeline. This test now does exactly the same as the first one. I removed it.

@tbosch
Copy link
Contributor

tbosch commented Sep 2, 2014

Could you add the following tests as well:

  • it('should not assign not parsable values to the scope if if allowInvalid is true'), test by using input[number] and assign a string that is not a number...

@@ -1633,6 +1633,24 @@ describe('input', function() {
'ng-model-options="{ getterSetter: true }" />');
});

it('should assign invalid values from $validators if allowInvalid is true', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

please change to: it('should assign invalid values to the scope if allowInvalid is true'...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

@tbosch
Copy link
Contributor

tbosch commented Sep 5, 2014

Note: I am in the process of refactoring ngModelController and formController to simplify control flow and eliminate bugs. See #8941.
This needs to wait until that other PR is in.

To be discussed in this PR:
Probably also remove ctrl.$$invalidModelValue and always write to $modelValue immediately (but don't write to the scope immediately)

@shahata
Copy link
Contributor Author

shahata commented Sep 5, 2014

np, i'll take care of it when rebasing

@shahata
Copy link
Contributor Author

shahata commented Sep 9, 2014

Rebased with #8941
I'll look into removing ctrl.$$invalidModelValue tonight

@Narretz
Copy link
Contributor

Narretz commented Sep 9, 2014

I have a PR for removing ctrl.$$invalidModelValue. I can put it up tonight. There is one thing that I'm not sure about, so I'd welcome your input

@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.1, 1.3.0-rc.2 Sep 9, 2014
This option allows to write invalid values to the model instead of having them become undefined.

Closes #8290
tbosch pushed a commit to tbosch/angular.js that referenced this pull request Sep 9, 2014
This option allows to write invalid values to the model instead of having them become undefined.
Use this together with calling `ctrl.$setValidity` directly for displaying errors
from serverside validation.

Closes angular#8290
Closes angular#8313
@tbosch tbosch closed this in 3c538c1 Sep 9, 2014
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014
This option allows to write invalid values to the model instead of having them become undefined.
Use this together with calling `ctrl.$setValidity` directly for displaying errors
from serverside validation.

Closes angular#8290
Closes angular#8313
tbosch pushed a commit that referenced this pull request Sep 10, 2014
This option allows to write invalid values to the model instead of having them become undefined.
Use this together with calling `ctrl.$setValidity` directly for displaying errors
from serverside validation.

Closes #8290
Closes #8313
Closes #9016
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014
This option allows to write invalid values to the model instead of having them become undefined.
Use this together with calling `ctrl.$setValidity` directly for displaying errors
from serverside validation.

Closes angular#8290
Closes angular#8313
mgallag pushed a commit to mgallag/angular.js that referenced this pull request Sep 10, 2014
This option allows to write invalid values to the model instead of having them become undefined.
Use this together with calling `ctrl.$setValidity` directly for displaying errors
from serverside validation.

Closes angular#8290
Closes angular#8313
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to write invalid $modelValues to scope
9 participants