fix(common): throw an error if trackBy is not a function #13420

DzmitryShylovich · 2016-12-13T10:25:33Z

vicb · 2016-12-13T19:38:48Z

modules/@angular/common/src/directives/ng_for.ts

-  @Input() ngForTrackBy: TrackByFn;
+  @Input()
+  set ngForTrackBy(fn: TrackByFn) {
+    if (typeof fn !== 'function') {


if (fn && typeof fn !== 'function') { }

Would be consistent with the differ:

constructor(private _trackByFn?: TrackByFn) { this._trackByFn = this._trackByFn || trackByIdentity; }

DzmitryShylovich · 2016-12-14T22:59:47Z

@vicb

if (fn && typeof fn !== 'function') {} Would be consistent with the differ

if I use this check instead of if (typeof fn !== 'function') it will be not possible to catch situation from the original issue *ngFor="let item of items; trackBy: item?.id". In this case trackBy is null == no error

DzmitryShylovich · 2016-12-16T09:51:00Z

every day http://prntscr.com/dk3j70

vicb · 2016-12-16T16:04:24Z

@mhevery @IgorMinar wdyt ? See the original issue also.

pkozlowski-opensource · 2016-12-16T16:15:37Z

modules/@angular/common/src/directives/ng_for.ts

-  @Input() ngForTrackBy: TrackByFn;
+  @Input()
+  set ngForTrackBy(fn: TrackByFn) {
+    console.log('input', fn);


console.log leftover.

it was for debugging purposes :)

pkozlowski-opensource · 2016-12-16T16:15:42Z

modules/@angular/common/src/directives/ng_for.ts

+  set ngForTrackBy(fn: TrackByFn) {
+    console.log('input', fn);
+    if (fn && typeof fn !== 'function') {
+      console.log('inside', fn);


console.log leftover.

pkozlowski-opensource · 2016-12-16T16:15:52Z

modules/@angular/common/test/directives/ng_for_spec.ts

+               `<template ngFor let-item [ngForOf]="items" [ngForTrackBy]="item?.id"></template>`;
+           fixture = createTestComponent(template);
+
+        console.log('1');


console.log leftover.

pkozlowski-opensource · 2016-12-16T16:16:44Z

modules/@angular/common/test/directives/ng_for_spec.ts

+        console.log('1');
+        fixture.detectChanges();
+           // expect(() => fixture.detectChanges())
+           //     .toThrowError(/trackBy must be a function, but received 1/);


expectation commented out...

pkozlowski-opensource · 2016-12-16T16:16:49Z

modules/@angular/common/test/directives/ng_for_spec.ts

+
+        console.log('1');
+           getComponent().items = [{id: 1}, {id: 2}];
+        console.log('1');


console.log leftover.

pkozlowski-opensource · 2016-12-16T16:18:34Z

I agree with @DzmitryShylovich that this is very common mistake people do and it fails silently so having some kind of check would improve ergonomics. Having said this I've removed "LGTM" label as beside the decision this PR needs serious cleanup :-)

Closes angular#13388

DzmitryShylovich · 2016-12-16T17:01:14Z

@pkozlowski-opensource

this PR needs serious cleanup

done 😄

vicb · 2016-12-16T18:17:23Z

We all agree that the check is needed here.

I should have given more context: The question here is should null be an error or not. I vote for no error as it would be a BC break and could be valid. @DzmitryShylovich thinks that null should be an error. Both POV are IMO acceptable but hopefully if null is caused by a wrong syntax, at some null will be defined and throw. Also IDE/linters should prevent this soon.

mhevery · 2016-12-17T00:21:24Z

This LGTM. I think we should throw if it is null

*ngFor="let i in is; trackBy: myFunction" if myFunction is null than reverting to identity is wrong.

) * fix(common): throw an error if trackBy is not a function Closes angular#13388 * refactor(platform-browser): disable no-console rule in DomAdapter

…ect usages of trackBy

…rect usages of trackBy

…ect usages of trackBy Added note about angular#13420 to the CHANGELOG, which affects incorrect usages of trackBy

…ch affects incorrect usages of trackBy

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

Reverts a breaking change introduced in 2.4.1 by #13420 fixes #13641

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

Reverts a breaking change introduced in 2.4.1 by #13420 fixes #13641

angular-automatic-lock-bot · 2019-09-10T12:20:54Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Dec 13, 2016

DzmitryShylovich force-pushed the gh/13388 branch 2 times, most recently from 8156b5e to 2e0dd9a Compare December 13, 2016 10:58

vicb reviewed Dec 13, 2016

View reviewed changes

vicb added area: common Issues related to APIs in the @angular/common package hotlist: error messages action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM labels Dec 13, 2016

DzmitryShylovich force-pushed the gh/13388 branch 4 times, most recently from a3636f9 to 6022416 Compare December 13, 2016 22:56

pkozlowski-opensource reviewed Dec 16, 2016

View reviewed changes

pkozlowski-opensource removed the pr_state: LGTM label Dec 16, 2016

fix(common): throw an error if trackBy is not a function

6c60fce

Closes angular#13388

DzmitryShylovich force-pushed the gh/13388 branch from 78d7865 to 6c60fce Compare December 16, 2016 16:25

refactor(platform-browser): disable no-console rule in DomAdapter

ba87788

mhevery added action: merge The PR is ready for merge by the caretaker pr_state: LGTM labels Dec 17, 2016

mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 17, 2016

chuckjaz merged commit fcd116f into angular:master Dec 21, 2016

DzmitryShylovich deleted the gh/13388 branch December 21, 2016 00:25

kuncevic mentioned this pull request Dec 22, 2016

trackBy must be a function, but received null swimlane/ngx-datatable#388

Closed

vandres pushed a commit to vandres/angular that referenced this pull request Dec 22, 2016

Added note about angular#13420 to the CHANGELOG, which affects incorr…

c1a407a

…ect usages of trackBy

vandres pushed a commit to vandres/angular that referenced this pull request Dec 22, 2016

Added note about angular#13420 to the CHANGELOG, which affects incor…

ed3d529

…rect usages of trackBy

vandres pushed a commit to vandres/angular that referenced this pull request Dec 22, 2016

Added note about angular#13420 to the CHANGELOG, which affects incorr…

795cc75

…ect usages of trackBy Added note about angular#13420 to the CHANGELOG, which affects incorrect usages of trackBy

vandres pushed a commit to vandres/angular that referenced this pull request Dec 22, 2016

Added note about angular#13420 to the CHANGELOG, which affects incorr…

4ad3d41

…ect usages of trackBy Added note about angular#13420 to the CHANGELOG, which affects incorrect usages of trackBy

vandres pushed a commit to vandres/angular that referenced this pull request Dec 22, 2016

docs(changelog): added note about angular#13420 to the CHANGELOG, whi…

231cc86

…ch affects incorrect usages of trackBy

vicb added a commit to vicb/angular that referenced this pull request Jan 3, 2017

fix(Common): allow null/undefined values for NgForTrackBy

82673da

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

vicb mentioned this pull request Jan 3, 2017

fix(Common): allow null/undefined values for NgForTrackBy #13762

Merged

vicb added a commit to vicb/angular that referenced this pull request Jan 4, 2017

fix(Common): allow null/undefined values for NgForTrackBy

f19692f

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

vicb added a commit to vicb/angular that referenced this pull request Jan 4, 2017

fix(Common): allow null/undefined values for NgForTrackBy

438f435

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

IgorMinar pushed a commit that referenced this pull request Jan 5, 2017

fix(Common): allow null/undefined values for NgForTrackBy

f88cd2f

Reverts a breaking change introduced in 2.4.1 by #13420 fixes #13641

IgorMinar pushed a commit to IgorMinar/angular that referenced this pull request Jan 6, 2017

fix(Common): allow null/undefined values for NgForTrackBy

80a389a

Reverts a breaking change introduced in 2.4.1 by angular#13420 fixes angular#13641

IgorMinar pushed a commit that referenced this pull request Jan 6, 2017

fix(Common): allow null/undefined values for NgForTrackBy

78c77bc

Reverts a breaking change introduced in 2.4.1 by #13420 fixes #13641

IgorMinar pushed a commit that referenced this pull request Jan 6, 2017

fix(Common): allow null/undefined values for NgForTrackBy

6be55cc

Reverts a breaking change introduced in 2.4.1 by #13420 fixes #13641

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(common): throw an error if trackBy is not a function #13420

fix(common): throw an error if trackBy is not a function #13420

DzmitryShylovich commented Dec 13, 2016 •

edited

vicb Dec 13, 2016

DzmitryShylovich commented Dec 14, 2016

DzmitryShylovich commented Dec 16, 2016 •

edited

vicb commented Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

DzmitryShylovich Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

pkozlowski-opensource commented Dec 16, 2016

DzmitryShylovich commented Dec 16, 2016

vicb commented Dec 16, 2016

mhevery commented Dec 17, 2016

angular-automatic-lock-bot bot commented Sep 10, 2019

fix(common): throw an error if trackBy is not a function #13420

fix(common): throw an error if trackBy is not a function #13420

Conversation

DzmitryShylovich commented Dec 13, 2016 • edited

vicb Dec 13, 2016

Choose a reason for hiding this comment

DzmitryShylovich commented Dec 14, 2016

DzmitryShylovich commented Dec 16, 2016 • edited

vicb commented Dec 16, 2016

pkozlowski-opensource Dec 16, 2016

Choose a reason for hiding this comment

DzmitryShylovich Dec 16, 2016

Choose a reason for hiding this comment

pkozlowski-opensource Dec 16, 2016

Choose a reason for hiding this comment

pkozlowski-opensource Dec 16, 2016

Choose a reason for hiding this comment

pkozlowski-opensource Dec 16, 2016

Choose a reason for hiding this comment

pkozlowski-opensource Dec 16, 2016

Choose a reason for hiding this comment

pkozlowski-opensource commented Dec 16, 2016

DzmitryShylovich commented Dec 16, 2016

vicb commented Dec 16, 2016

mhevery commented Dec 17, 2016

angular-automatic-lock-bot bot commented Sep 10, 2019

DzmitryShylovich commented Dec 13, 2016 •

edited

DzmitryShylovich commented Dec 16, 2016 •

edited