feat(ngModelOptions): support submit trigger

[shahata]

This is another attempt to solve #7017 from a different direction. This one actually moves a big part of the ngModelOptions mechanism out of the various input directives and into ngModelController which is a better approach in my opinion.

[mary-poppins]

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 (#7116)

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!

[petebacondarwin]

@shahata - where did we get to on this? I am back from vacation and would like to move this forward.
I need to take a proper look at this code. I'll probably do so Thursday now.

[shahata]

@petebacondarwin - cool, let me know what you think. There wasn't any progress on this while you've been away :)

[petebacondarwin]

I had a chat with the team about form submit stuff today. They are happy to get this in but wanted the event to be "private", i.e. not a public API and should be called $$formSubmit, only for our consumption at this stage.

I have a slightly uneasy feeling about this second implementation, regarding the fact that certain events always update the pendingValue even if they are not on the updateOn list. It may be nothing but I need to think about this a bit more.

But generally I like the idea of removing duplication.

[shahata]

Sure. I'll update the PR to make the $$formSubmit event private. BTW, I like this PR better not only because it makes implementing an input directive that support $options much less complicated and repetitive, but also because it will make it very easy to add validateOn option if it gets merged.

[shahata]

done

[petebacondarwin]

I don't think we actually need to use events for this form submit stuff! The FormController already has access to all child inputs, because their NgModelControllers add them to any enclosing form. So we can just use that to "broadcast" to the child NgModelControllers.

[shahata]

You are right, I decided to leave the event just so users will be able to trigger the submit flow regardless of the form submit. Since we decided to make this event private, it probably doesn't make sense to keep using an event. I updated the PR accordingly and now we use a $$formSubmit method instead of an event.

[IgorMinar]

is this really needed?

[petebacondarwin]

When I remove it the test fails...

[petebacondarwin]

OK, so the problem is that the first digest is synchronizing the pendingValue to the value of the model. If you don't have a digest before the first "change" event to update the pendingValue then when the first digest is called it doesn't realise that pendingValue had already been set and overrides it with undefined. See https://github.com/shahata/angular.js/blob/updateOnSubmit2/src/ng/directive/input.js#L1869-L1872

if (ctrl.$viewValue !== value) {
  ctrl.$viewValue = pendingValue = value;
  ctrl.$render();
}

I think it might be fair to expect a digest always to occur before any "change" event can occur on the input...?

[shahata]

Exactly, the first digest cycle needs to happen before anything is changed by the user, otherwise it will just reset the $viewValue to the current value of the model, overriding any change that was already done by the user. This is the case in ngModelOptions regardless of this PR, it is just emphasized by the use of pendingValue.

I think it is a not only fair to expect this - more over, I believe that if we don't do this in tests it is kind of "cheating" since it isn't realistic that the digest doesn't happen before the change.

[IgorMinar]

I see. That makes sense. We should then move the digest call into the compileInput helper method.

[shahata]

You are right, I will do this in a separate commit.

[shahata]

@IgorMinar - Thanks for your comments. I've updated the PR accordingly.

[IgorMinar]

I'm proposing these changes:

• let's keep the semantics of $setViewValue and when $viewValue is updated as it used to be
• let's create a new variable on the model controller called $$lastCommitedViewValue or something with a similar name and use this to store the last value that was successfully propagated to modelView
• let's change the $$realSetViewValue to a method that will sync the current viewValue with the modelValue by running the parser/validator pipeline (this would be extracted from the flush method we have) (called $commitViewValue?)
• we can then get rid of the $$formSubmit method on ngModelController and replace it with ngFormController calling this sync method from the form's formSubmit method

optionally it might then make sense to:

• rename the $cancelUpdate to rollback or something else
• rename ngModel's updateOn property to commitOn

[petebacondarwin]

So effectively compared to the current PR:

$viewValue becomes $lastCommitedViewValue
pendingValue becomes $viewValue

[petebacondarwin]

$cancelUpdate ($rollbackViewValue) would just reset the $viewValue to $lastCommittedViewValue and call $render.

When a debounce resolves (or a non-debounced sync event occurs), it just calls $commitViewValue()

$commitViewValue will send $viewValue through the $parsers to update the $modelValue and the $lastCommitedViewValue will be updated

When the form is submitted, then FormController will just call $commitViewValue

[shahata]

@IgorMinar @petebacondarwin - Thanks! I've just pushed another commit with your proposed changes. I hope I understood correctly what you meant. :) The whole thing looks much clearer now in my opinion.

The only problem I had with your suggestions is with getting rid of $$formSubmit in ngModelController. The problem is that ngFormController invokes this method on all of its controls - some of them are ngModelController, but some may be nested form controllers.

This means that the name of the method needs to be the same in ngModelController and ngFormController. We can rename the method on ngFormController to be $$commitViewValue, but if we decide to leave it as $$formSubmit, it means that we need $$formSubmit also in ngModelController.

Also, I think I like updateOn better than commitOn, but I don't have a strong opinion on this... :)

[petebacondarwin]

I would go with changing FormController's method to $commitViewValue. I think we decided that we would not use the $$ on this method. It makes more sense to "commit the value of the form" than to "submit a form on an input"

[shahata]

done

[petebacondarwin]

Looks good to me. I don't like the name of $$debouncedCommitViewValue method. How about $$handleViewValueEvent, since that is what it is doing (from the outside)?

[petebacondarwin]

I agree about the updateOn in ng-model-options. Changing it to commitOn would just confuse people who are not aware of the internal workings of NgModelController.

[petebacondarwin]

Alternatively, $$debouncedCommitViewValue could be $$debounceViewChange since that is what it is responsible for.

[shahata]

Great! Renamed to $$debounceViewChange.

[IgorMinar]

we should also cancel any pending task with $timeout.cancel(pendingDebounce);

[shahata]

right, adding

[IgorMinar]

@petebacondarwin I was about to suggest switching form controller to use $commitViewValue as well, then I refreshed the diff and the change was already there. magical moment ;-)

[IgorMinar]

there is a typo through out this diff. $$lastCommitedViewValue should be $$lastCommittedViewValue (note t -> tt)

[IgorMinar]

To summarize, we are now exposing apis with these names:

ngModelOptions:

• updateOn property

NgModelController:

• $setViewValue method
• $commitViewValue method
• $rollbackViewValue
• $$debounceViewChange
• $$lastCommittedViewValue

FormController:

• $commitViewValue

I'd like to unify the names if possible because now we are using two concepts to refer to the same thing: 1/ "update" and 2/ "commit". Actually there is also 3/ "change" for "$$debounceViewChange".

If we were to stick with things as they are we should at least rename $$debounceViewChange to $$debounceViewValueCommit.

Going beyond that can we somehow unify the "update" and "commit" names so that we use only one or the other but not both?

Some suggestions:

• $commitViewValue method -> rename to $flushViewValueUpdate, $performViewValueUpdate ???
• $rollbackViewValue -> rename to $cancelViewValueUpdate ???
• $$debounceViewChange -> rename to $$debounceViewModelFlush

[IgorMinar]

other than these small things this change looks great

[IgorMinar]

(oh and we need a better commit message that summarizes the changes and describes breaking changes if there are any, but let's resolve the other issues first)

[shahata]

Renamed $$debounceViewChange to $$debounceViewValueCommit + fixed the $timeout bug and $$lastCommitedViewValue typo. Modifying updateOn to commitOn will have to wait for tomorrow :)

[shahata]

The current commit message is just a placeholder of course. Currently can't think of any breaking changes, but I'm possibly missing something.

[petebacondarwin]

I know @IgorMinar was not keen on $updateModelValue as a method but if we were to live with it then things could be quite consistent:

ngModelOptions:

• updateOn property ----> updateOn (stays same)

NgModelController:

• $setViewValue method ----> $setViewValue (stays same)
• $commitViewValue method ----> $updateModelValue
• $rollbackViewValue ----> $resetViewValue
• $$debounceViewChange ----> $$debounceUpdateModelValue
• $$lastCommittedViewValue ----> $$previousViewValue

FormController:

• $commitViewValue ----> $updateModelValue

In this configuration, the view value can be set and reset, while update is a single concept which refers to the model value being updated to match the parsed current view value, which can be debounced if necessary.

[shahata]

@IgorMinar @petebacondarwin - I rebased and updated the comments on my commits. I believe we should separate this to three different commits. Let me know if you prefer I squash all three into one.

[petebacondarwin]

Landed as a0ae07b