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

Fix for #8234 #8687

Closed
wants to merge 1 commit into from
Closed

Fix for #8234 #8687

wants to merge 1 commit into from

Conversation

alexeykudinkin
Copy link

This is crappy synchronisation bug that can run you absolutely crazy trying to devise what has gone wrong with your ngMinLength.

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@Narretz
Copy link
Contributor

Narretz commented Aug 21, 2014

Thanks for the PR, but please stay constructive. So you're saying the bug happens because the validators get reattached to the ngModel every time the element is recreate by ngIf, for example? If this is true, then the problem should affect other validators too, right? When I looked at this in #8234, it looked like the root cause might lie somewhere else. In any case, this needs a test.

@Narretz Narretz added this to the 1.3.0 milestone Aug 21, 2014
@caitp
Copy link
Contributor

caitp commented Aug 21, 2014

+1 @Narretz

@alexeykudinkin we can't accept pull requests without test cases (it's actually very hard to review this without a test case showing what is going wrong).

@matsko I wonder if your refactoring might solve this --- if we get a test case for it which is failing in master, should definitely add it to the refactor

@alexeykudinkin
Copy link
Author

Sorry, didn't mean anything in person.

@alexeykudinkin
Copy link
Author

@caitp, actually i wondered that there were (hardly) any tests for those type of the directive.

@matsko
Copy link
Contributor

matsko commented Aug 21, 2014

Yes it should fix it. Sorry guys for not replying here earlier to keep everyone posted.

@alexeykudinkin
Copy link
Author

@Narretz this is pure racing problem caused by validators being assigned (and sequentially fired) right before attr observer being attached.

@alexeykudinkin
Copy link
Author

@matsko could you, please, point out the place fixing this. Scanned through your PR (https://github.com/angular/angular.js/pull/8267/files#diff-3), but hardly can find any change that may fix that.

@matsko
Copy link
Contributor

matsko commented Aug 21, 2014

The big refactoring is local at the moment. Lots of broken tests and changes. I can paste a link here at the end of the day tomorrow.

@pandamouse
Copy link

But what about ng-maxlength for input type="number" ? value.length fails at the moment....?

@matsko
Copy link
Contributor

matsko commented Aug 29, 2014

@pandamouse that fix is in the works here: #7968

@matsko
Copy link
Contributor

matsko commented Aug 29, 2014

@alexeykudinkin we pushed a fix for minlength/maxlength that does this. Can you test on master and reply here if it breaks? We will reopen it if it's still broken.

@matsko matsko closed this Aug 29, 2014
@alexeykudinkin
Copy link
Author

No, it's not fixed.

@alexeykudinkin
Copy link
Author

I've pushed a test to show off a PoC to clarify what i see is wrong.

1b6b1aa

Test indicates that model value is actually being validated twice: first during angular bootstrap (when no minlength attribute being observed yet (boom!) and after minlength attribute observation (being triggered explicitly by observer itself).

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.

7 participants