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

feat(ngModel) Allow running the formatters without a change to the modelValue #10764

Closed
wants to merge 1 commit into from

Conversation

realityking
Copy link
Contributor

This is needed when a formatter can be configured in some way and needs to be rerun after the change. Issue propped up for me when I tried to fix UI Bootstrap with Angular 1.3 (angular-ui/bootstrap#2659)

Fixes #3407

@Narretz
Copy link
Contributor

Narretz commented Jan 16, 2015

It's a simple change and I think we should merge this. Let's see what @petebacondarwin says.
However, I don't think $runFormatters should call $$runValidators. We have too much magic happen in ngModel already, where stuff triggers other stuff. If someone wants validation after calling `$runFormatters, that's what $validate is for (which also has some additional checks).

@Narretz
Copy link
Contributor

Narretz commented Jan 16, 2015

Oh, since ngModel is planned to change quite a lot in 1.4, it probably only makes sense if we merge this into 1.3.x, too

@petebacondarwin
Copy link
Contributor

I would prefer the new API to be of the form: $setModelValue(newModelValue), which is symmetric with the $setViewValue(newViewValue) on the other end of the pipeline. To get the same functionality as in $runFormatters() you could simply call:

ngModel.$setModelValue(ngModel.$modelValue);

Would this cause any problems if it updates the $$rawModelValue too?

@realityking
Copy link
Contributor Author

@Narretz I've removed the validation from $runFormatters.
I'd be great if this could make it into 1.3. I was planning on duplicating the code in UI Bootstrap but this way I wouldn't have to.

@petebacondarwin If it would be done this way, shouldn't only $$rawModelValue be set and all following steps in the pipeline be run? However I think there would still be value in a method that only formats the value, especially now that the validators aren't run. Maybe the new method should be called $format to be more consistent with $validate?

@Narretz
Copy link
Contributor

Narretz commented Jan 19, 2015

@petebacondarwin I like the idea to make it analogous to $setViewValue() - in that case, though we should run the validators in the end, to keep it consistent. I don't think it's a problem that the $$rawModelValue is set, as long as the value inside $setModelValue() corresponds to the value on the scope. But same as with $setViewValue, it's the dev's responsibility to guarantee this.

@realityking I don't think $format is like $validate, it's really like the non-existent $runParsers function. Would $setModelValue work for your use case?

@realityking
Copy link
Contributor Author

@Narretz I can changed it to $setModelValue. If it can be guaranteed, that formatters are always run after calling it, that this will cover my use case. I'll write the documentation after I get a general ok on the API.

ctrl.$setModelValue(5);
expect(ctrl.$render).not.toHaveBeenCalled();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Other tests to consider:

  • that the formatters are run
  • that the validators are run
  • what happens if something goes wrong (say a formatter fails) to the modelValue/viewValue/validity
  • do the formatters/validators run if the modelValue has not actually changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the API is ok, most tests from the next section (model -> view) could be moved here. model -> view basically only needs to tests that it calls $setModelValue if the model value has changed.

@petebacondarwin
Copy link
Contributor

Also what is the relationship between the model expression on the scope and the $modelValue after running $setModelValue... Will the next digest change the model value back?

@realityking
Copy link
Contributor Author

@petebacondarwin that's a good question, I'll test it to make sure

@realityking
Copy link
Contributor Author

@petebacondarwin I finally got back to this. Indeed the value is override on the next digest. (I've added a test for this)

I've managed to work around it, but I'm not sure if the fromModel argument should be exposed as part of the API. What do you think?

@joeenzminger
Copy link

I ran into this thread while researching a similar problem. My question about adding something to the API would be "why doesn't ngModel watch for changes in formatters and do this on it's own (without adding an API call)?".

@realityking
Copy link
Contributor Author

@joeenzminger While nice from DX perspective (however such an API would still be necessary if something internal to the formatter changes), I don't think it's worth adding two watchers for each ngModel.

@joeenzminger
Copy link

I guess it revolves around how expensive running the formatters is. I don't think it would require adding an extra watch, but it would make the existing watch more expensive (you'd have to run the formatters each digest to see if something internal or external changed). But I see your point.

@realityking
Copy link
Contributor Author

@petebacondarwin Any news on this?

@petebacondarwin
Copy link
Contributor

I will take a look this week.

@petebacondarwin petebacondarwin self-assigned this Apr 12, 2015
@petebacondarwin petebacondarwin modified the milestones: 1.4.0-rc.1, 1.4.x - jaracimrman-existence, 1.5.x - migration-facilitation Apr 12, 2015
@realityking
Copy link
Contributor Author

@petebacondarwin ping

expect(ctrl.$modelValue).toBe(10);
});

it('should update the value on the scope', inject(function($compile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually looking at this again I don't like that we can set the $modelValue to a value that is different to the scope.

For your use case @realityking you should always call this method as ngModel.$setModelValue(ngModel.$modelValue) and providing a different value should be documented as not supported.
This would remove the need for this test and the associated behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the follow up.

This sounds like a bad API (you can call this function only with this value, nothing else). To be honest, given this comment, I'd be more comfortable returning to $runFormatters (maybe called $format to be more inline with the other methods in ngModelController)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I guess you are right.
If we had ngModel.$formatModel, then I suspect we would need to have a complementary ngModel.$parseView? Since changes to the formatters might well include changes to the parsers too? In which case one would want the new viewValue to be parsed and update the model....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a decent idea, even if only for API symmetry reasons. (I believe you can already gain the effect by calling '$setViewValue` with the current view value)

I'll try to get it done over the weekend.

@petebacondarwin
Copy link
Contributor

I am going to be away from next week: @Narretz could you take over this one?

@realityking
Copy link
Contributor Author

Just a small FYI, I've been working on this a bit but haven't got it done yet. Maybe I get to it this weekend.

@ghost
Copy link

ghost commented Mar 16, 2016

Any progress with this?

@rwoloszyn
Copy link

Is this going to be merged ?

@rubenswieringa
Copy link

It would be great to have this functionality in there – @petebacondarwin / @realityking, do you have any plans to finish the last bit of this PR so it can be merged?

@Narretz Narretz modified the milestones: 1.5.x, 1.6.x Mar 8, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 21, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 25, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 25, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 26, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 26, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 27, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 27, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 27, 2017
Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 29, 2017
@Narretz Narretz closed this in 05fdf91 Sep 29, 2017
Narretz added a commit that referenced this pull request Sep 29, 2017
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.

NgModel needs a way to manually re-run the formatter chain
7 participants