Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expression has changed after it was checked #6

Open
lskrajny opened this issue Feb 13, 2017 · 25 comments
Open

Expression has changed after it was checked #6

lskrajny opened this issue Feb 13, 2017 · 25 comments

Comments

@lskrajny
Copy link

lskrajny commented Feb 13, 2017

Ocassionally, getting similiar errors:

Expression has changed after it was checked. Previous value: '16 minutes ago'. Current value: '17 minutes ago'.

I believe it is related to the pipe relaying on timer to support real-time scenario.
Is it possible to disable real-time updates?

@AndrewPoyntz
Copy link
Owner

AndrewPoyntz commented Feb 13, 2017

Interesting, it should play fairly nicely with the change detector, would be great to see a plunker or something if possible?

disabling real-time updates is not there at the moment, but it's definitely something which could be added though! (#7)

@nPn-
Copy link

nPn- commented Mar 20, 2017

I just want to add that I am also seeing these errors pop up.

@cmargulhano
Copy link

I'm too

@plul
Copy link

plul commented Jul 5, 2017

I'm having this issue too.

Here is an example of the error thrown:

ERROR Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'a minute ago'. Current value: '2 minutes ago'.
    at viewDebugError (core.es5.js:8633)
    at expressionChangedAfterItHasBeenCheckedError (core.es5.js:8611)
    at checkBindingNoChanges (core.es5.js:8775)
    at checkNoChangesNodeInline (core.es5.js:12329)
    at checkNoChangesNode (core.es5.js:12303)
    at debugCheckNoChangesNode (core.es5.js:12922)
    at debugCheckRenderNodeFn (core.es5.js:12862)
    at Object.eval [as updateRenderer] (LoggingPage.html:26)
    at Object.debugUpdateRenderer [as updateRenderer] (core.es5.js:12844)
    at checkNoChangesView (core.es5.js:12125)
    at callViewAction (core.es5.js:12487)

It is possible that this only happens when Angular is not in prod mode.

@cmargulhano
Copy link

In my case, in prod mode, these errors do not stop, but we cannot see, since console.log is disabled.
I changed code of the pipe to include change detection and the problem stopped happening. See below:

        this.timer = this.ngZone.runOutsideAngular(() => {
            if (typeof window !== 'undefined') {
                return window.setTimeout(() => {
                    this.changeDetectorRef.detectChanges(); // <<<<<<<<----HERE
                    this.ngZone.run(() => this.changeDetectorRef.markForCheck());
                }, timeToUpdate);
            }
            return null;
        });

@plul
Copy link

plul commented Jul 6, 2017

@cmargulhano I appreciate you working on and sharing a potential fix to this issue!

However, I don't quite get the fix. I would be thankful if you could shed some light on it.

Is there a need for both lines?

this.changeDetectorRef.detectChanges();
this.ngZone.run(() => this.changeDetectorRef.markForCheck());

And why do you not run detectChanges inside the ngZone?

@cmargulhano
Copy link

detectChanges() : void ---->>> to checks the change detector and its children:

What it means if there is a case where any thing inside your model has changed but it has not reflected the view, you might need to notify Angular to detect those changes and update the view.

I think in this case, where you update the model after the changeDetection cycle is finished,will get this error:

"Expression has changed after it was checked";

To avoid this we must use: this.changeDetectorRef.detectChanges();

What do you think about it?

One thing is certain: the problem is not happening anymore.

@cmargulhano
Copy link

Sorry folks, the error still occurs, I'm looking for the solution but I think that's the way.

@cmargulhano
Copy link

Yes @plul we could try this:

            return window.setTimeout(() => {
                this.ngZone.run(() => {
                    this.changeDetectorRef.detectChanges();
                    this.changeDetectorRef.markForCheck();
                });
            }, timeToUpdate);

@cmargulhano
Copy link

Why you are using ngZone?
Why not use simple setTimeout like that:

        this.timer = window.setTimeout(() => {
            this.changeDetectorRef.detectChanges();
        }, timeToUpdate);

I'm testing this right now.
I will let know my results.

@plul
Copy link

plul commented Jul 7, 2017

@cmargulhano I agree. I cannot see a reason to run the setTimeout outside the ngZone to begin with.

However, I still do not have something that works. This does not work:

      this.timer = window.setTimeout(() => {
        this.changeDetectorRef.markForCheck();
        this.changeDetectorRef.detectChanges();
      }, timeUntilUpdate);

@plul
Copy link

plul commented Jul 7, 2017

@cmargulhano You mentioned earlier that these errors do not stop when Angular is in production mode - only you cannot see them because console.log is disabled.

How then do you know that these errors still occur? What symptom are you seeing?

@plul
Copy link

plul commented Jul 7, 2017

How about this:

We modify this repo into a pure pipe that takes a time string like 2017-07-04T12:01:32Z and returns an Observable. The Observable starts to emit strings like 'a few seconds ago', 'a minute ago', etc., as necessary.

Then we use Angular's async pipe to subscribe to it. Which means we end up with something like
<div>{{ myTimestamp | timeAgo | async }}</div>

@plul
Copy link

plul commented Jul 7, 2017

I propose the following. It works perfectly in my code so far. I hope you will help me to test it further.

Use it with Angulars async pipe:
<div>{{ myTimestamp | timeAgo | async }}</div>

It may be possible to directly calculate when the next value should be emitted, instead of repeatedly testing for a new string. But I don't think there is much to gain in terms of performance anyway. This should be much better in terms of performance, than the impure pipe we started out with.

import { Pipe, PipeTransform, NgZone } from "@angular/core";
import { Observable } from 'rxjs/Observable';
import { Observer } from 'rxjs/Observer';

interface processOutput {
  text: string; // Convert timestamp to string
  timeToUpdate: number; // Time until update in milliseconds
}

@Pipe({
  name: 'timeAgo',
  pure: true
})
export class TimeAgoPipe implements PipeTransform {

  constructor(private ngZone: NgZone) { }

  private process = (timestamp: number): processOutput => {
    let text: string;
    let timeToUpdate: number;

    const now = new Date();

    // Time ago in milliseconds
    const timeAgo: number = now.getTime() - timestamp;

    const seconds = timeAgo / 1000;
    const minutes = seconds / 60;
    const hours = minutes / 60;
    const days = hours / 24;
    const months = days / 30.416;
    const years = days / 365;

    if (seconds <= 10) {
      text = 'now';
    } else if (seconds <= 45) {
      text = 'a few seconds ago';
    } else if (seconds <= 90) {
      text = 'a minute ago';
    } else if (minutes <= 50) {
      text = Math.round(minutes) + ' minutes ago';
    } else if (hours <= 1.5) {
      text = 'an hour ago';
    } else if (hours <= 22) {
      text = Math.round(hours) + ' hours ago';
    } else if (hours <= 36) {
      text = 'a day ago';
    } else if (days <= 25) {
      text = Math.round(days) + ' days ago';
    } else if (months <= 1.5) {
      text = 'a month ago';
    } else if (months <= 11.5) {
      text = Math.round(months) + ' months ago';
    } else if (years <= 1.5) {
      text = 'a year ago';
    } else {
      text = Math.round(years) + ' years ago';
    }

    if (minutes < 1) {
      // update every 2 secs
      timeToUpdate = 2 * 1000;
    } else if (hours < 1) {
      // update every 30 secs
      timeToUpdate = 30 * 1000;
    } else if (days < 1) {
      // update every 5 mins
      timeToUpdate = 300 * 1000;
    } else {
      // update every hour
      timeToUpdate = 3600 * 1000;
    }

    return {
      text,
      timeToUpdate
    };
  }

  public transform = (value: string | Date): Observable<string> => {
    let d: Date;
    if (value instanceof Date) {
      d = value;
    }
    else {
      d = new Date(value);
    }
    // time value in milliseconds
    const timestamp = d.getTime();

    let timeoutID: number;

    return Observable.create((observer: Observer<string>) => {
      let latestText = '';

      // Repeatedly set new timeouts for new update checks.
      const registerUpdate = () => {
        const processOutput = this.process(timestamp);
        if (processOutput.text !== latestText) {
          latestText = processOutput.text;
          this.ngZone.run(() => {
            observer.next(latestText);
          });
        }
        timeoutID = setTimeout(registerUpdate, processOutput.timeToUpdate);
      };

      this.ngZone.runOutsideAngular(registerUpdate);

      // Return teardown function
      const teardownFunction = () => {
        if (timeoutID) {
          clearTimeout(timeoutID);
        }
      };
      return teardownFunction;
    });
  }

}

@cmargulhano
Copy link

cmargulhano commented Jul 7, 2017 via email

@fiznool
Copy link

fiznool commented Jul 10, 2017

@plul just stopped here to say thanks very much for that code snippet, I've used it as the basis of a timeago pipe which is translatable with @ngx-translate/core. Thanks!

@plul
Copy link

plul commented Jul 11, 2017

@cmargulhano @fiznool I'm happy to hear it! Thanks. :)

@AndrewPoyntz
Copy link
Owner

@plul I'm more than happy to change to that approach if it works! Feel free to PR it :)

(Thanks @cmargulhano for looking into this too! 👍 )

@plul
Copy link

plul commented Jul 20, 2017

@AndrewPoyntz Thanks, it would be great to see it merged!

Though I would love to contribute further, at the moment I am too busy at work dealing with a bunch of other issues preventing us from launching our app into production.

In the mean time, I welcome you to update the repo on your own, if you so choose.

That would require updating the readme, as well as updating or scrapping your test spec.

You'll also note that I took the liberty of redefining some of the update intervals and strings that it displays to suit my own needs, however those could easily be changed back to what you have if you wish.

The major version number should probably be bumped too, in accordance with the semantic versioning guidelines, as requiring the async pipe is not backwards compatible.

@edenworky
Copy link

Over a year later, I'm still getting these errors. Is this being worked on? Seems a simple merge

@yuezhizizhang
Copy link

I'm still meeting the same errors.

@Art2058
Copy link

Art2058 commented Aug 28, 2018

@plul
good!!

@VhMuzini
Copy link

This error persist

@HannaBabrouskaya
Copy link

Any updates on this?

@mateotherock
Copy link

@plul Thank you! I was looking for alternatives to an impure pipe using moment.humanize in my application and came across this thread and your code works perfectly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests