Fix provider global clenup on error #89
Conversation
| const { rerender } = render(<App />); | ||
| rerender(<App />); | ||
|
|
||
| expect(children.mock.calls[0][0]).toBe(children.mock.calls[1][0]); |
There was a problem hiding this comment.
an interesting test, look like there is a story behind it.
There was a problem hiding this comment.
This is because context provider memo can be fiddly 😅
| } | ||
|
|
||
| componentWillUnmount() { | ||
| globalDi._remove(this.props.use); |
There was a problem hiding this comment.
these are not the same use you've got during the initial render and later used for _fromProvider, these are different.
There were a few bugs due to mismatches of this sort, I would better remeber a "cleanup callback" somewhere in getValue
There was a problem hiding this comment.
How come? I would expect them to be consistent 🤔 Unless someone decides to conditionally add/remove injectables (which we call out as not supported) it should contain the same "stuff"
There was a problem hiding this comment.
You know, stuff happens.
This is not a real problem, but a potential.
Ideally just expose another (secret) method to check if global DI is clear. Then we can call it before test to verify that everything is as it should be. Later we can remove this method.
Well, it should be fine, but we all know that our assumptions are not always true
|
|
||
| componentDidCatch(err) { | ||
| globalDi._remove(this.props.use); | ||
| throw err; |
There was a problem hiding this comment.
Just to double check - :notsureif: unmount not happens during catch.
I have a very string dejavu feeling right now, like we had this conversation already...
I just cannot imagine that some allocated resources (any) can be "not freed" in case of emergency. That would like a disaster!
There was a problem hiding this comment.
Not, just tested: neither useEffects cleanup nor componentWillUnmount will fire if a child throws during render (just checked). 😢
There was a problem hiding this comment.
Something we need to clarify for ourselves
Closes #88