salesforce / lwc

⚡️ LWC - A Blazing Fast, Enterprise-Grade Web Components Foundation
https://lwc.dev
Other
1.63k stars 395 forks source link

[error-boundary] Parent's errorCallback not invoked when child gets into a bad state during its errorCallback #1169

Closed ravijayaramappa closed 4 years ago

ravijayaramappa commented 5 years ago

Description

When a child component gets into a bad state during its errorCallback phase, the engine fails to invoke the parent's errorCallback. In addition to this, the child component is not disconnected from the dom

Steps to Reproduce

<!-- parent -->
<template>
     {state.title}   <br/>
     <c-child></c-child>
</template>

import { LightningElement, track } from 'lwc';
export default class App extends LightningElement {
    @track 
    state = { title: 'initial' };
    errorCallback(error) {
        this.state.title = error.message;
    }
}
<!-- child -->
<template>
    I am a healthy child who will throw in the errorCallback handler
    <template if:true={state.error}>
        <c-grand-child></c-grand-child>    
    </template>
    <c-grand-child></c-grand-child>
</template>

import { LightningElement, track } from 'lwc';

export default class Child extends LightningElement {
    @track state = {error: false};
    errorCallback() {
        this.state.error = true;
    }
}
<!-- grand child -->
import { LightningElement } from 'lwc';

export default class GrandChild extends LightningElement {
    renderedCallback() {
        throw new Error('Grand child throws in renderedCallback');
    }
}

https://playground.lwcjs.org/projects/BS1lIVahh/10/edit

Expected Results

c-parent's errorCallback to have been invoked. nodes must be disconnected from the dom. Also node must be disconnected(I think?)

Actual Results

Screen Shot 2019-04-11 at 2 10 21 PM

Automated test at https://github.com/salesforce/lwc/pull/1166/files#diff-016034a5086083569b4fac4d57f2cbffR114

apapko commented 5 years ago

This is fixed: https://playground.lwcjs.org/projects/iYOBY8WzV/5/edit When child throws, its content will be removed. However, the element itself will remain in the DOM. This covers recovery scenario where parent passes a new prop to the child and allows it to recover and render a healthy view. The only time the child element is not going to be in the DOM is when child throws in the constructor.

pmdartus commented 4 years ago

While working on https://github.com/salesforce/lwc/pull/1704, I realized that this issue has not been solved. There is still a test referencing this issue that has is disabled and failing: https://github.com/salesforce/lwc/blob/master/packages/integration-karma/test/component/LightningElement.errorCallback/index.spec.js#L114-L115

git2gus[bot] commented 4 years ago

This issue has been linked to a new work item: W-7181638

pmdartus commented 4 years ago

I spent some time investigating this issue. And I came to the conclusion that the behavior described in this issue matches the mental model I have with error boundaries.

IMO, the test is wrong, the parent component should never know about the child failing to render the alternative view because the child errorCallback handles both cases (actual view and alternative view errors).

Thoughts?

apapko commented 4 years ago

I agree with @pmdartus that the parent component shouldn't know about child's failing. However, what i don't like with the current implementation is that the parent component, by default, assumes a healthy child, unless its errorCallback is invoked. In other words, it creates an illusion that everything in the descendant's tree is healthy. The general expectation around errorCallback, is that if the child fails to recover its offending descenders, then the handling should be delegated to the parent. Unfortunately, as @pmdartus has pointed out, the second failure is not recovered because there is nothing to rehydrate. This means that the responsibility to handle a dual failure scenario falls onto the component author. This case should be documented as it is not obvious that the recover won't occur.

Should the test case be removed, as per @pmdartus suggestion? cc @caridy

SasidharanA commented 2 years ago

FYR: https://sfdcsasid.blogspot.com/2022/06/errorcallback-in-lwc.html