Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I started out with a couple of performance optimisations:
===
first in case they are reference-equal, which also lets us remove some comparisons if either of the values are primitive or nullfor
loop, which avoids allocatingObject.entries
for the array (which is potentially expensive)Object.keys
, which is cheaper thanObject.entries
.useDeepCompareMemoize
for the previous state, which uses an incrementing number as a signal to trigger rerenders (apparently this is faster 🤷♂️ faster kentcdodds/use-deep-compare-effect#37, fix: effect not called with getDerivedStateFromProps kentcdodds/use-deep-compare-effect#42)On balance I then felt like we should just add a dependency on use-deep-compare-effect; it's stable, tiny, fairly widely used, and depends only on dequal, which is also stable, tiny, and very widely used. I feel like the benefit of not having to test that functionality outweighs the drawbacks of adding a dependency, especially because neither of those packages have needed a release for over 2 years. This did mean we had to bump our minimum supported React version to be in line with that of use-deep-compare-effect, but I don't think that's too bad.
I wasn't able to eliminate the
ts-expect-error
- the problem is something I've run into repeatedly in this SDK. There's a good summary of it here.I tweaked the
AsyncResult
type to allow type narrowing (if you check for loading, and then error, you don't still have to then check whether data is undefined). I also added anaborted
flag so that we don't update the state if a new effect has been triggered since we started, or the component has unmounted. In the future we might want to addsignal: AbortSignal
to theRequestOptions
type to support actually aborting in-flight requests in the gRPC and HTTP clients, but that's beyond the scope for now I reckon!