angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
96.22k stars 25.49k forks source link

docs: LifeCycle - changes in AfterViewChange -> expression has changed after it was checked. #10131

Closed Tragetaschen closed 3 years ago

Tragetaschen commented 8 years ago

As requested by @vicb,

I'm submitting a ... (check one with "x") continuation of

[x ] bug report
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

6005. Let's try to keep this one short so it isn't closed prematurely…

Current behavior Changing a binding in ngAfterViewInit throws an exception.

Expected/desired behavior It shouldn't.

Reproduction of the problem If the current behavior is a bug or you can illustrate your feature request better with an example, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (you can use this template as a starting point: http://plnkr.co/edit/tpl:AvJOMERrnz94ekVua0u5).

http://plnkr.co/edit/v9zf4Txp8tOeUV35DdMD?p=preview

What is the expected behavior? Still, it shouldn't

What is the motivation / use case for changing the behavior? To make it so it doesn't throw an exception

Please tell us about your environment:

@vicb mentioned "actions" that "have been taken". If these exist, then they are not available in rc.4.

zoechi commented 8 years ago

I think that's exactly what the error message is about - when changes happen during change detection - and ngAfterViewInit is called by change detection.

You can explicitly notify about the change using ChangeDetectorRef.detectChanges() http://plnkr.co/edit/nHTxb8ENxrvWIbBbfrJW?p=preview

If this is actually supposed to work without an exception it would be great to get confirmation otherwise the documentation should be explicit about it.

Tragetaschen commented 8 years ago

This whole thing is really confusing:

What makes ngAfterViewInit so special? Neither ngOnInit nor ngAfterContentInit have this behavior, so doesn't any kind of property setter. You mention the concept of "during change detection". What's that, when do I care and how do I detect that?

This thing looks a lot like $digest() and if (!$scope.$$phase)

DzmitryShylovich commented 8 years ago

http://stackoverflow.com/questions/34364880/expression-has-changed-after-it-was-checked/34364881#34364881

Tragetaschen commented 8 years ago

This doesn't explain anything. It just states the status quo and shows another way of delay-updating the binding (an observable for a one-time update—seriously?)

vicb commented 8 years ago

@zoechi is right here, you are not supposed to update the context after it has been dirty checked (change detection)

What makes ngAfterViewInit so special? Neither ngOnInit nor ngAfterContentInit have this behavior, so doesn't any kind of property setter.

ngAfterViewInit runs after the change detection. Which is not the case for ngOnInit. ngAfterContentInit is executed after the content has been dirty checked.

You mention the concept of "during change detection". What's that, when do I care and how do I detect that?

That's the error message you have.

I don't think this is an issue but probably deserve better doc.

/cc @wardbell

Tragetaschen commented 8 years ago

There is documentation for that: https://angular.io/docs/ts/latest/guide/lifecycle-hooks.html#!#wait-a-tick

The thing is, if you look through angular-framework-colored glasses ("that's how it is, so it's not a bug"), this is likely the only implementation possible for the given set of problems it tries to solve. However, I haven't seen a proper explanation why this is actually the case and why the alternatives are worse than the number of people constantly tripping over that detail. I ran into this mostly accidentally, when I moved stuff from ngOnInit to ngAfterViewInit, because I needed to access an ElementRef. Now I have to learn that instead of doing whatever I want in ngOnInit (called once for my component at the beginning of its lifetime) there is some non-obvious, hard-to-circumvent exception when I'm doing the same in ngAfterViewInit (called once for my component at the beginning at the beginning of its lifetime).

Couldn't ngAfterViewInit get a parameter (that should be a non-breaking change) applyChanges: (work: () => void) => void (or a typedef…) so you can applyChanges(()=>this.member = "a");? That would hide the actual ugliness (like calling setTimeout) behind the scenes and give an opportunity to drop some documentation close to the AfterViewInit interface.

In addition (even without the extra parameter), you could extend the exception message with something like "Did you try to update bound values in ngAfterViewInit or ngAfterViewChecked?" and link to some documentation. I don't know how many other common cases trigger this Exception, but at least you get to the learning quicker.

vicb commented 8 years ago

Couldn't ngAfterViewInit get a parameter ...

That would hurt perf by running the change detection twice. Note that you can still do that by injecting the ChangeDetectionRef in your component but that's not something that should be promoted.

you could extend the exception message ...

Yep, that's probably something we should do: have a better doc and link it form the exception

Tragetaschen commented 8 years ago

Couldn't ngAfterViewInit get a parameter

That would hurt perf

Huh? The function that's passed as the parameter would be the equivalent of

function applyChanges(work: ()=>void) {
  setTimeout(work, 0);
}

and you would only call that when you actually want to change bindings / members. It's just hiding the various possible workarounds that all already trigger a second round of change detection. This function would do it in the "most efficient" way, maybe even platform specific.

vicb commented 8 years ago

This function would do it in the "most efficient" way, maybe even platform specific.

a setTimeout() would trigger a change detection of the whole app which is far from being efficient.

Tragetaschen commented 8 years ago

I've got the feeling I'm missing something fundamentally:

As far as I understand things for ngAfterViewInit, the only way to change the context is by triggering a second round of change detection and all workarounds do that one way or the other.

If that's not the case, hopefully I'm going to learn something next. If that's the case, the proposed parameter would just make that necessary second round of change detection accessible without jumping through too many hoops.

pkozlowski-opensource commented 8 years ago

As far as I understand things for ngAfterViewInit, the only way to change the context is by triggering a second round of change detection.

Correct.

and all workarounds do that one way or the other

Those workarounds are only necessary since people fail to see the single-pass, uni-directional data flow is a "good thing" bringing the following benefits:

Now, as soon as we start to introducing params that say "hey, just run me another round of checks" we are back to Angular 1.x algorithm with all its drawbacks:

It is kind of funny to see that people want uni-directional data flow benefits without its less exciting properties :-) Yes, uni-directional data flow makes your app better but you need to think about its structure slightly differently.

The thing is, if you look through angular-framework-colored glasses ("that's how it is, so it's not a bug")

It is not angular-specific thing. The same principle will apply to any framework that is enforcing single-pass uni-directional data flow.

Not sure what could be done with this issue. What you are proposing would be a fundamental change in a design decision. The current design is not arbitrary but rather is a response to problems that were very apparent with multiple rounds of change detection. Maybe you should rather expose your functional use case so others can suggest the best approach.

In any case this should be closed, IMO.

Tragetaschen commented 8 years ago

Hmm, I can definitely see your point, although I still think your arguments are quite harsh for the only-once-ngAfterViewInit. And it still doesn't really explain why ngAfterContentInit and earlier hooks don't suffer from that within the same component. It could well be called outside change-detection and trigger another round (that means exactly one and if and only if the hook actually exists), but I'm not at all familiar with the current implementation, so that might be wishful thinking…

My use case is doing some layout calculations (getBoundingClientRect()) based on a @ViewChild member for other parts of the component.

I think improving the exception message / documentation is something that should be done to close this (I promise to remain silent :) ).

slmyers commented 8 years ago

I think you could leverage the async pipe couples with the Observable interface to meet your usecase.

//our root app component
import {Component, AfterViewInit} from '@angular/core'
import {BehaviorSubject} from 'rxjs/BehaviorSubject'

@Component({
  selector: 'my-app',
  providers: [],
  template: `
    <div>
      <h2>Hello {{name$ | async}}</h2>

    </div>
  `,
  directives: []
})
export class App implements AfterViewInit {
  public name$: any = new BehaviorSubject("john");

  public ngAfterViewInit() {
    setTimeout(() => {
      this.name$.next('Angular2 (Release Candidate!)')
    }, 0);
  }
}

plunker

Tragetaschen commented 8 years ago

I have looked at the (generated) source code for calling ngAfterContentInit.

AFAI can see, it's entirely possible to change the semantics of the hook. Currently, it's called directly guarded by if ((self.numberOfChecks === 0)) as the last step during change detection. Instead of the direct function call within the guard, the compiler could generate scheduling a new task that calls the function and then runs a second (and only a second!) change detection.

There is also the possibility of introducing a new lifecycle hook that has the above, new semantic of scheduling a task and thus is invoked "after the dust has settled".

@pkozlowski-opensource Reading again your comment, I can see a number of things that don't belong to this discussion given the current implementation or are just exaggerated

So there is the current implementation of ngAfterViewInit

The changed semantics for ngAfterViewInit

If a component doesn't implement the hook, then there is no perf implication at all because the compiler would not generate that code.

Needless to say, I'm pro changed semantics, because it's so much easier to develop against and the perf implications are small and local. I've tried to find actual implementations of ngAfterViewInit on Github, but it's mostly trivial implementations (console.log) or wrong implementations (the initialized class member luckily has the same value as what's set through ngAfterViewInit). material2 or ng-bootstrap both succeed in avoiding this hook altogether as does the entire angular repository (besides integration tests).

zoechi commented 8 years ago

For me a mention of required manual change detection turn in the ngAfterViewInit docs would be enough.

kemsky commented 8 years ago
  1. Message is not clear enough, it reports only top-level component and it reports only value, i.e. true != false and that is all, no name. Today I've spent hours trying to find changing property, by the way i could not reproduce it on local machine (inconsistent behavior for the same build on different pc, chrome). It was caused by NgClassValid in internal component which rendered select options with ngfor.
  2. Users should have listener to do initialization, there is no way to avoid it. Denial is not a solution. I bet everyone will end up using setTimeout because there is NO other way to do that. It makes every performance argument invalid. @Tragetaschen explained it very well. Also i have to mention that amount of breaking changes is already outside of any acceptable range.
CumpsD commented 8 years ago

is there a way to easily find out the name of the offender? as @kemsky noted, there's only the value right now

Dozer1170 commented 8 years ago

I am having issues with this error as well. My issue occurs when adding another element to an ngFor collection, which also instantiates 2 layers of child components. The only way I have found to debug it is to comment out lines of code or lines of html until the error doesn't happen anymore and then surround in a setTimeout. I have not been able to resolve any of these errors with ChangeDetectorRef.detectChanges() but setTimeout seems to work consistently.

This message is what I get from angular. The error is actually in a completely different component, and it can also be caused by an ngIf check so it can be very time consuming to track down.

borrower.component.html:0:0 caused by: Expression has changed after it was checked. Previous value: 'false'. Current value: 'true'.

+1 for outputting the property name in the error.

Edit: Sometimes the binding can be a combination of multiple properties so it may be harder than expected to output. Ex: *ngIf="model.booleanOne && model.booleanTwo"

patrickmichalina commented 7 years ago

I think I am experiencing the same thing now with the following error:

Expression has changed after it was checked. Previous value: 'CD_INIT_VALUE'. Current value: 'message'. It seems like the view has been created after its parent and its children have been dirty checked. Has it been created in a change detection hook ?

I have 2 levels of dynamically rendered components, one nested within the other. After applying the setTimeout hack, the things appear to work...

lednhatkhanh commented 7 years ago

Me too, any news on this?

tuongaz commented 7 years ago

My current solution is use ChangeDetectionStrategy.OnPush and use Obseravble + async.

MaximeMorin commented 7 years ago

I had the same issue. I want to disable my component during a REST API call; the double checks weren't returning the same value due to the call returning in-between. In the end, I just disabled this double check for the application. (Definitively not the best solution, but it works ... for now...)

app.component.ts: let applicationRef2:any = this.applicationRef; // Injected in the consturctor. applicationRef2['_enforceNoNewChanges'] = false;

tytskyi commented 7 years ago

@MaximeMorin take a look at this.changeDetectorRef.detach() https://angular.io/docs/ts/latest/api/core/index/ChangeDetectorRef-class.html This should be proper solution for your use-case

born2net commented 7 years ago

I had the same issue with ngSwitch, moved out of ngAfterInit and into constructor which solved my issue, but smells funny :)

 template: `
        <small class="debug">{{me}}</small>
        <ul [ngSwitch]="m_blockTypeSelected">
            <div *ngSwitchCase="'BLOCKCODE_IMAGE'">
                <h1>Image</h1>
            </div>
            <div *ngSwitchCase="'LABEL'">
                <h1>label</h1>
            </div>
            <div *ngSwitchDefault>
                <h1>def</h1>
            </div>
        </ul>
    `,

...

constructor(private yp: YellowPepperService, private rp: RedPepperService, private bs: BlockService, private hp:HelperPepperService) {
        super();
        // console.log(this.bs.getServiceType());

        this.yp.listenBlockChannelSelected()
            .mergeMap((i_campaignTimelineChanelPlayersModel: CampaignTimelineChanelPlayersModel) => {
                return this.bs.getBlockData(i_campaignTimelineChanelPlayersModel.getCampaignTimelineChanelPlayerId())
            }).subscribe((blockData: IBlockData) => {
            this.m_blockTypeSelected = blockData.blockType;
        }, (e) => console.error(e))

    }
patrickmichalina commented 7 years ago

In my particular case (doubly nested dynamic components), this was fixed by simply making sure an object was defined (ex: empty array or object) before any ngLifecyckeHooks are called.

just-jeb commented 7 years ago

Same issue here (similar to @kemsky and @Dozer1170). I'm creating components with *ngFor and these components affect the parents' validity. Components generation (with *ngFor) is triggered by parent change detector, so the child component is being generated and initialized during the parent detection cycle. This causes the NgClassValid of parent to change "because of" change detection cycle. This behavior (child components initialized during the *ngFor of parent component) seems completely fine to me and I see no reason to wrap the initialization in setTimeout(0). Will it be fixed sometime or I should just use the setTimeout(0) workaround?

klinki commented 7 years ago

I'm trying to write component for table header with sorting functionality. I'm getting into trouble when data changes and I need to update output inside setter.

I basically have something like this:

    @Input()
    public set data(inputData: any[]) {
        this._originalData = inputData || [];
        this.sortData();
        this.sortedDataChange.emit(this.sortedData);
    }

and of course, it leads to famous exception

Expression has changed after it was checked.

I also ended up using setTimeout(0) workaround. It is quite ugly and if it really causes change detection in the whole application, then it is wrong. Is there any better way how to do that? Preferably way which would trigger change detection only on related places?

ps2goat commented 7 years ago

I hate to compare apples to oranges, but ASP.NET webforms had the lifecycle strategy, as well, but it also had an event where you could run any last-second changes (OnPreRender). After the prerender events, no changes would affect what was being returned to the client.

https://msdn.microsoft.com/en-us/library/ms178472.aspx#additional_page_life_cycle_considerations

I haven't dug into the framework code for Angular, so I don't know how this would fit in, but it would be nice to have. In one case, we have dynamic modals cleaned up in the 'ngAfterViewChecked' method, and we get the mentioned error.

Maybe we need more event hooks?

kemsky commented 7 years ago

Well, actually angular has some information that could be helpful:

function debugCheckDirectivesFn(view, nodeIndex, argStyle)

I really doubt that it would take a lot of time to implement at least hint with component name.

wardbell commented 7 years ago

Should add to LifeCycle guide that changes in AfterViewChanges will trigger this exception. Mention the setTimeout workaround. Yes that triggers a new round of change detection for the entire page. Too bad.

just-jeb commented 7 years ago

@wardbell I personally managed to replace all my setTimeout workarounds with changeDetector.detectChanges. This is the right way to do (the Angular way) and it should be stated in the documentation. I have to mention though it was very hard to understand where this error is coming from and on which component I actually need to trigger the change detection. I think there is a place for improvement in error message that is being thrown.

kemsky commented 7 years ago

@meltedspark good idea, but unfortunately very inconvenient to implement, since you need to inject ChangeDetector every time. I used another approach, all components that need setTimeout trick put callbacks into queue, queue uses single setTimeout/Promise.resolve().then to execute pending callbacks, so effectively only one change detection round is triggered.

public callLater(callback:Callback):void {
        if(this.callbacks === null) {
            this.callbacks = [];
            Promise.resolve().then(() => {
                const callbacks = this.callbacks;
                this.callbacks = null;
                for(let callback of callbacks) {
                    callback();
                }
            });
        }
        this.callbacks.push(callback);
}
just-jeb commented 7 years ago

@kemsky Don't get me wrong, the timeOut is totally fine from the execution perspective. But it is indeed a workaround, while changeDetector.detectChanges (even though you need to inject that) is a proper way of work when working with Angular. This is why I think it should be stated in official Angular documentation.

NgxDev commented 7 years ago

@pkozlowski-opensource

Those workarounds are only necessary since people fail to see the single-pass, uni-directional data flow is a "good thing" bringing the following benefits:

I have this issue in a carousel, because I need to set some property on the carousel item component, property which is only known after being computed in the parent carousel component. And all the carousel-items are coming through ng-content, so I can only work with the items in the ngAfterContentInit of the parent carousel component. Is this not uni-directional? I believe it is. Not sure about the "single-pass", though :) But very so often we do need to set some properties which aren't yet known in ngOnInit. And introducing a service and a subject here (like many other people have suggested, not necessarily in this board nor for my specific case) for the carousel-item component to subscribe to, and using that to emit something from the carousel component... well, for my case I think it would be ridiculous, since a carousel can have a lot of items. If I have 100 items in a carousel, I'd have 100 active subscriptions. Not to mention more than 1 carousel.

@tytskyi

take a look at this.changeDetectorRef.detach() https://angular.io/docs/ts/latest/api/core/index/ChangeDetectorRef-class.html This should be proper solution for your use-case

I've tried to inject ChangeDetectorRef and use detach() (on both components: the carousel component and the carousel-item child component - basically on an entire tree), but I still get the error. It's as if Angular doesn't care that we've disabled change detection or not, I believe it's not taking that into account before throwing the error. Am I missing something? I would love to hear that I do! :D

constructor(private cdr: ChangeDetectorRef) {
  this.cdr.detach();
}

I'm inclining to believe that, since the error isn't thrown in a production build (because there's no additional change detection ran), I should simply leave it like that, without using any setTimeout() hack (yes, I will call setTimeout() a hack in over 99% of the cases) and just learn to live with some red text in the console during development :)... ok, I could probably never do that, it bugs me too much

k1r0s commented 7 years ago

There is an oficial answer from angular team to "where should I place my execute first logic on a component"?

mdimp commented 7 years ago

ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'wrtwrt'. Current value: 'efgh'.

my json value is changed after it loaded I tryed Onint ngAfterviewInit but I didnt get my actual json value is changed . can any one fix this

this is my json "headers" : "[{\"key\":\"wrtwrt\",\"value\":\"abcd\"},{\"key\":\"efgh\",\"value\":\"abcd\"}]",

angular 2 code this.profile = data; for(let i= 0; i < this.profile.length; i++) { let stringAray = JSON.parse(this.profile[i].headers); console.log('stringAray', stringAray); this.profile = stringAray; }

mlc-mlapis commented 7 years ago

@maddela ... not without a simple plunker which should demonstrate the problem.

valera-rozuvan commented 6 years ago

I recently stumbled on this issue also. Do you need a fresh plnkr to help you reproduce this issue? Or is the cause known, and we are waiting for a fix?

pacurtin commented 6 years ago

Really wish the Angular team would see how much of usability issue this is for the average developer. Burying their heads in the sand and making excuses isn't making my unit tests go green.

jenniferfell commented 6 years ago

docsarea=lifecyclehooks docsarea=changedetection

maxkoretskyi commented 6 years ago

@Tragetaschen

What makes ngAfterViewInit so special? Neither ngOnInit nor ngAfterContentInit have this behavior, so doesn't any kind of property setter.

because it's triggered after Angular checked the binding and recorded new value, the two other hooks are triggered before the rendering part when the binding is checked. See this article that explains the order of hooks and why this is important Everything you need to know about the ExpressionChangedAfterItHasBeenCheckedError error

jelbourn commented 3 years ago

Closing since this the new error guides cover this error with deeper explanation

angular-automatic-lock-bot[bot] commented 3 years ago

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.