feat(nfts): add nfts owned to redux #3902
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3902 +/- ##
==========================================
- Coverage 82.27% 82.26% -0.01%
==========================================
Files 708 710 +2
Lines 26289 26342 +53
Branches 3584 3584
==========================================
+ Hits 21630 21671 +41
- Misses 4596 4608 +12
Partials 63 63
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/nfts/saga.ts
Outdated
| Accept: 'application/json', | ||
| }, | ||
| }) | ||
| if (!response.ok) yield put(fetchNftsFailed({ error: 'Could not fetch NFTs' })) |
There was a problem hiding this comment.
Shouldn't we throw here? And the catch will handle it.
Maybe we can include some data from the failed response in the message. I think response.status and response.statusText are nice to have.
Also, it's a good practice to always include the { }.
There was a problem hiding this comment.
The structure was recommended in #3908 (comment). I'll add in the brackets.
There was a problem hiding this comment.
i think you should throw here, it will land in the catch block so you don't need to duplicate the error handling (calling fetchNftsFailed twice)
# Conflicts: # src/redux/migrations.test.ts # src/redux/migrations.ts # src/redux/store.test.ts # src/redux/store.ts # src/web3/networkConfig.ts # test/schemas.ts
Description
Adds users owned NFTs to redux to allow for image cache hits: enabled via feature flag.
Test plan
Related issues
Backwards compatibility
Yes