Closed bholloway closed 2 years ago
@jackbrown not specifically and I have not checked that.
This is to fix a conceptual correctness problem - IMO the promise
should resolve data
or error
and never stale data
. Specifically update(() => null)
to delete an item should not keep a reference to data in the promise.
In react the promise
resolved value should not really be used, there are better ways for observing resource data
. However in playing with some "dependent resources" implementation outside react I find the promise is needed as an indicator of future value.
Since I started the branch I think this correctness with the promise
was partly addressed during the LRU changes so ultimately there was not a lot to do. Although I was tempted to make other fixes to tests etc which would be good to land. All that is to say we should discuss offline when/how these changes should land.
I'll drop this because of the conflict with the browser-only change.
Good discussion about race conditions with refresh. I will raise a separate PR with a more refined approach.
Best reviewed commit at a time.
Primarily I want to ensure that the resource
promise
will always resolvedata
or rejecterror
. The main culprit beingupdate()
where thepromise
becomes immediately mismatched with thedata
.Timeouts should also be rejecting errors IMO but are not included here. The logic to do that is not really more complicated but there is a more general problem that
expiresAt
andpromise
need to be set during hydration rather than being mutated on first access. I attempted this refactor but ultimately got stuck on the tests.Additionally in the remote resource fetch
I did a little work to make the tests more consistent but there is still a lot that needs doing there.