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

fix($compile): avoid calling $onChanges() twice for NaN initial values #15098

Closed
wants to merge 1 commit into from
Closed

fix($compile): avoid calling $onChanges() twice for NaN initial values #15098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3500,7 +3500,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
});

function recordChanges(key, currentValue, previousValue) {
if (isFunction(destination.$onChanges) && currentValue !== previousValue) {
if (isFunction(destination.$onChanges) && currentValue !== previousValue &&
// eslint-disable-next-line no-self-compare
(currentValue === currentValue || previousValue === previousValue)) {
Copy link
Contributor

@petebacondarwin petebacondarwin Sep 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

But this will not trigger a call to $onChanges if we go from a real number to NaN, no?

Sorry I forgot about the currentValue !== previousValue check before it

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think a test of that situation would be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

Landed but forgot to add this extra test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add the extra test. It is good excuse to fix an indentation issue 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference: c0cbe54

Copy link
Contributor

Choose a reason for hiding this comment

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

You wouldn't believe how much time I spent fixing up the indentation in my conflict editor :-)

// If we have not already scheduled the top level onChangesQueue handler then do so now
if (!onChangesQueue) {
scope.$$postDigest(flushOnChangesQueue);
Expand Down
34 changes: 34 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4327,6 +4327,40 @@ describe('$compile', function() {
});


it('should not call `$onChanges` twice even when the initial value is `NaN`', function() {
var onChangesSpy = jasmine.createSpy('$onChanges');

module(function($compileProvider) {
$compileProvider.component('test', {
bindings: {prop: '<', attr: '@'},
controller: function TestController() {
this.$onChanges = onChangesSpy;
}
});
});

inject(function($compile, $rootScope) {
var template = '<test prop="a" attr="{{a}}"></test>' +
'<test prop="b" attr="{{b}}"></test>';
$rootScope.a = 'foo';
$rootScope.b = NaN;

element = $compile(template)($rootScope);
$rootScope.$digest();

expect(onChangesSpy).toHaveBeenCalledTimes(2);
expect(onChangesSpy.calls.argsFor(0)[0]).toEqual({
prop: jasmine.objectContaining({currentValue: 'foo'}),
attr: jasmine.objectContaining({currentValue: 'foo'})
});
expect(onChangesSpy.calls.argsFor(1)[0]).toEqual({
prop: jasmine.objectContaining({currentValue: NaN}),
attr: jasmine.objectContaining({currentValue: 'NaN'})
});
});
});


it('should only trigger one extra digest however many controllers have changes', function() {
var log = [];
function TestController1() { }
Expand Down