AndrewPoyntz / time-ago-pipe

An Angular pipe for converting a date string into a time ago
MIT License
130 stars 67 forks source link

Expression has changed after it was checked #6

Open lskrajny opened 7 years ago

lskrajny commented 7 years ago

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 commented 7 years ago

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- commented 7 years ago

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

cmargulhano commented 7 years ago

I'm too

plul commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

cmargulhano commented 7 years ago

Yes @plul we could try this:

            return window.setTimeout(() => {
                this.ngZone.run(() => {
                    this.changeDetectorRef.detectChanges();
                    this.changeDetectorRef.markForCheck();
                });
            }, timeToUpdate);
cmargulhano commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Cool man. Congratulations! Nice work. I will test! All my best, Cláudio

2017-07-07 13:16 GMT-03:00 Asger Juul Brunshøj notifications@github.com:

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:

{{ myTimestamp | timeAgo | async }}

import { Pipe, PipeTransform, ChangeDetectorRef, 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 changeDetectorRef: ChangeDetectorRef, ) { }

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 => { 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;
});

}

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AndrewPoyntz/time-ago-pipe/issues/6#issuecomment-313726956, or mute the thread https://github.com/notifications/unsubscribe-auth/ALFuGu3KPj1HrEUPncSoSnsLbbvyYeD-ks5sLln4gaJpZM4L_eL8 .

fiznool commented 7 years ago

@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 commented 7 years ago

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

AndrewPoyntz commented 7 years ago

@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! :+1: )

plul commented 7 years ago

@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 commented 6 years ago

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

yuezhizizhang commented 6 years ago

I'm still meeting the same errors.

Art2058 commented 6 years ago

@plul good!!

VhMuzini commented 6 years ago

This error persist

HannaBabrouskaya commented 5 years ago

Any updates on this?

mateotherock commented 5 years ago

@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.