chore(deps): Use typed-redux-saga #3961
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3961 +/- ##
==========================================
- Coverage 83.21% 83.18% -0.04%
==========================================
Files 711 711
Lines 26232 26248 +16
Branches 3580 3584 +4
==========================================
+ Hits 21830 21835 +5
- Misses 4340 4352 +12
+ Partials 62 61 -1
Continue to review full report in Codecov by Sentry.
|
|
I think codecov is having trouble understanding the yield* generator format |
| @@ -66,11 +66,7 @@ exports[`CircleButton renders correctly with minimum props 1`] = ` | |||
| xmlns="http://www.w3.org/2000/svg" | |||
| > | |||
| <path | |||
| d="M2.141,89.13c1.425,1.429,3.299,2.142,5.167,2.142c1.869,0,3.742-0.713,5.167-2.142l33.591-33.592L79.657,89.13 | |||
| c1.426,1.429,3.299,2.142,5.167,2.142c1.867,0,3.74-0.713,5.167-2.142c2.854-2.854,2.854-7.48,0-10.334L56.398,45.205 | |||
There was a problem hiding this comment.
these three snaps are the same, just different indentations.
There was a problem hiding this comment.
curious why these changed?
There was a problem hiding this comment.
It was this which isn't needed. They'll go back to the way they were :) https://github.com/valora-inc/wallet/pull/3961/files#r1270625689
| return !dek ? null : hexToBuffer(dek) | ||
| const dek: string = yield* call(accountsWrapper.getDataEncryptionKey, accountAddress) | ||
| yield* put(updateAddressDekMap(accountAddress, dek || null)) | ||
| return dek | ||
| } | ||
|
|
||
| export function* doFetchDataEncryptionKey(walletAddress: string) { |
There was a problem hiding this comment.
This function was previously returning null | string | Buffer. Various places that used it usually assumed it was either string or buffer. I updated it to always return null | string
There was a problem hiding this comment.
Do the dependencies of this assume non null? If so, do they need to be updated to handle nulls?
There was a problem hiding this comment.
no, the dependencies assumed it could be nulll
|
Nice 🙌 ! |
There was a problem hiding this comment.
Looks mostly good. Nice job!
Few things:
- So with this PR we get typings for sagas, but lose code coverage because jest doesn't work with yield* right?
- I started leaving comments on removing explicit types for yield* call, but seems like its removed only in a few places, so feel free to ignore them. But eventually I guess we can remove the explicit types since they all should be inferred now
- Do we need to specify the type for
yield* takealways? - There are a few places where we do
as unknown as typewithout any comments, are these the same as the bug you mentioned in some places?
babel.config.js
Outdated
| @@ -34,6 +34,7 @@ module.exports = { | |||
| '@babel/plugin-transform-flow-strip-types', // for a bug in FlatList caused by adding @babel/plugin-proposal-class-properties | |||
| '@babel/plugin-proposal-class-properties', // for ethers | |||
| '@babel/plugin-transform-private-methods', // for ethers | |||
| 'macros', | |||
There was a problem hiding this comment.
maybe add a comment similar to other entries here on why this is required?
There was a problem hiding this comment.
Why is it needed?
I looked at https://github.com/celo-tools/celo-web-wallet/blob/master/.babelrc and https://github.com/Uniswap/wallet/blob/main/apps/mobile/babel.config.js they both use typed-redux-saga but I can't yet find this there as well.
There was a problem hiding this comment.
It seems it may be unnecessary.
Not that both the projects listed above import like this:
import { call, put } from 'typed-redux-saga'instead of
import { call, put } from 'typed-redux-saga/macro'like we did in this PR.
Could we do the same?
Maybe this will help with the code coverage issue too?
There was a problem hiding this comment.
Answering myself, after re-reading the README for https://github.com/agiledigital/typed-redux-saga 😄
This gives you all the benefits of strong types during development without the overhead induced by all the calls to typed-redux-saga's proxies.
Not entirely sure about the overhead of not using macros but it seems it would be quite lite.
If this helps with code coverage, I think we should probably favor not using macros.
What do you think?
There was a problem hiding this comment.
Without macros all of our redux-saga-test-plan tests were failing with various issues. I couldn't get it to work. See branch here: #3991
There was a problem hiding this comment.
oh thanks, I found a way to make it work: #4002
Shown on a single test, but I think all tests should be able to work similarly.
There was a problem hiding this comment.
Awesome! Thanks @jeanregisser , I'll try to get it working on this PR.
| return !dek ? null : hexToBuffer(dek) | ||
| const dek: string = yield* call(accountsWrapper.getDataEncryptionKey, accountAddress) | ||
| yield* put(updateAddressDekMap(accountAddress, dek || null)) | ||
| return dek | ||
| } | ||
|
|
||
| export function* doFetchDataEncryptionKey(walletAddress: string) { |
There was a problem hiding this comment.
Do the dependencies of this assume non null? If so, do they need to be updated to handle nulls?
There was a problem hiding this comment.
Epic work!!
Can't express my gratitude enough ❤️
Left a couple of small comments.
Looks like we can drop a few more types, but no big deal.
Too bad about the coverage changing. It seems it's caused by the automatic replacement of yield* via the macros.
Maybe this will get fixed with an upgrade to jest? I'm not too hopeful though.
Update: see #3961 (comment)
src/firebase/notifications.ts
Outdated
| case NotificationTypes.PAYMENT_RECEIVED: | ||
| yield call(handlePaymentReceived, message.data as unknown as TransferNotificationData) | ||
| handlePaymentReceived(message.data as unknown as TransferNotificationData) |
There was a problem hiding this comment.
Why aren't we turning it to yield* call?
There was a problem hiding this comment.
handlePaymentReceived is not an async function
correct
No, only for channels that don't specify a type. Some, like the WalletConnectActions channel do that.
most of them are. I've updated with more comments |
Description
Using the typed-redux-saga package to bring type checking to our sagas. This is in preparation for bumping our typescript version.
Test plan
Updated unit tests
Related issues
Preparing for https://linear.app/valora/issue/ACT-784/typed-contracts-with-typechain