Replace ancestorComponent with vnode parent pointer (+5 B)#1688
Conversation
| diffed?(vnode: VNode): void; | ||
| /** Attach a hook that is invoked after an error is caught in a component but before calling lifecycle hooks */ | ||
| catchError?(error: any, component: Component): void; | ||
| catchError?(error: any, vnode: VNode): void; |
There was a problem hiding this comment.
wondering if there would be a use for vnodes here
There was a problem hiding this comment.
Honestly, I don't think there really is... I'm not super happy with this change. I have more thoughts about this. Will post more soon
There was a problem hiding this comment.
Personally, I don't mind the change for now. It could prove useful in the future for preact/debug. We could theoretically catch top-level errors and display a nice message in the console.
There was a problem hiding this comment.
#1692 contains where my brain went when making this change. Let me know what you think!
There was a problem hiding this comment.
Awesome, marking it as private is an even better solution 👍
JoviDeCroock
left a comment
There was a problem hiding this comment.
Personally I really like this change, it's more intuitive reasoning about a parentVnode (having 1 depth difference) instead of an ancestorComponent (having x difference). I'm not misunderstanding this I hope?
Great work Andre!
| * Unmount a virtual node from the tree and apply DOM changes | ||
| * @param {import('../internal').VNode} vnode The virtual node to unmount | ||
| * @param {import('../internal').Component} ancestorComponent The parent | ||
| * @param {import('../internal').VNode} parentVNode The parent |
There was a problem hiding this comment.
The comment here still refers to a "parent component" rather than a vnode. Can the parent VNode be obtained via the vnode._parent link here instead of passing an argument?
There was a problem hiding this comment.
Thanks! I'll update the comment. Since this function is recursive, I wanted to keep parentVNode as a pointer to first parent that isn't being unmounted, thinking it should handle errors of children that are being unmounted (should an unmounting error boundary still handle errors for children as they are unmounted?). However, let me check what React does here... perhaps we can save some bytes?
There was a problem hiding this comment.
Hmm seems React does swallow errors if the nearest error boundary to a thrown error is being unmounted (https://codesandbox.io/s/react-throwing-unmount-1vdvn) (<- could someone double check me on this?)
I think I'll leave our behavior for now just cuz I think I'd prefer to make noise on errors (e.g. failing to unsubscribe some resource which could lead to a memory leak).
There was a problem hiding this comment.
Agree, swallowed errors is always painful to debug 👍
| describe('#componentDidCatch', () => { | ||
|
|
||
| /** @type {Error} */ | ||
| let expectedError; |
There was a problem hiding this comment.
@rpetrich FYI, I've updated the error boundary tests a bit. Lemme know if you have any thoughts!
There was a problem hiding this comment.
I think the biggest change is specifying that throwing refs can be caught in the component that defines them
There was a problem hiding this comment.
Updated error boundary tests look good and check more behaviour 👍
marvinhagemeister
left a comment
There was a problem hiding this comment.
I really like this change as it makes upwards traversal a lot more intuitive and will make it easier to fix the remaining Fragment issues for good. Thank you so much for the PR 👍 💯
|
To check my understanding, does every vnode now have a parent pointer, regardless of type (dom, component or fragment)? This means to get the ancestor component, it might be necessary to follow several parent pointers up the tree until we get to a component node? |
robertknight
left a comment
There was a problem hiding this comment.
This looks good to me. The effort to normalize the vnode tree structure makes sense.
|
@robertknight yup, your comment describes the changes perfectly 👍 |
Summary
This is pre-req for #1689 (which fixes #1593, #1604, #1605).
As a side benefit, it continues to normalize our VNode structure to make our virtual tree easier to reason and work with.
2b087d5 Replace ancestorComponent with parent vnode pointer (+8 B)
52f2161 Move _depth property to VNode (-3 B)
Breaking changes
catchErrorandcatchRendernow take VNodes as their arguments instead of Components