feat(23012): convert 2 level 19 files to ts#23276
feat(23012): convert 2 level 19 files to ts#23276DDDDDanica wants to merge 3 commits intodevelopfrom
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
e75f733 to
5ffd231
Compare
5ffd231 to
2d08eed
Compare
Builds ready [2d08eed]
Page Load Metrics (968 ± 428 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #23276 +/- ##
===========================================
+ Coverage 68.67% 68.74% +0.07%
===========================================
Files 1106 1106
Lines 43356 43307 -49
Branches 11591 11573 -18
===========================================
- Hits 29773 29769 -4
+ Misses 13583 13538 -45 ☔ View full report in Codecov by Sentry. |
2d08eed to
4245085
Compare
Builds ready [4245085]
Page Load Metrics (1188 ± 397 ms)
|
shared/modules/transaction.utils.ts
Outdated
| try { | ||
| ({ name } = data && parseStandardTokenTransactionData(data)); | ||
| const parsedData = data && parseStandardTokenTransactionData(data); | ||
| if (parsedData && parsedData.name) { |
There was a problem hiding this comment.
a lot better for readability 👍🏼
Builds ready [00694ce]
Page Load Metrics (1521 ± 396 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
shared/modules/transaction.utils.ts
Outdated
| // not need to re-establish the type. | ||
| let inferrableType = txMeta.type; | ||
| if (INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type) === false) { | ||
| if (!INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type as TransactionType)) { |
There was a problem hiding this comment.
Let's resolve this by letting the compiler narrow the type (+ adding a runtime check) instead of relying on a type assertion.
| if (!INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type as TransactionType)) { | |
| if (txMeta.type && !INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type)) { |
This added null check removes the | undefined from the type with runtime guarantees.
The type assertion is dangerous because it fixes the entire type as TransactionType, rather than only removing a portion of the type signature. For example, if we defined a TransactionMetaType somewhere down the road, the as TransactionType assertion would prevent the compiler from inferring that type, while a null check or non-nullable assertion would not.
There was a problem hiding this comment.
Thanks for the advice, will add ! see 5d8c13
shared/modules/transaction.utils.ts
Outdated
| isHexString(transactionMeta?.txParams?.maxFeePerGas as string) && | ||
| isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas as string) |
There was a problem hiding this comment.
We could guarantee that undefined is never passed into isHexString by providing fallback values that won't pollute the arguments' type signatures. This provides runtime safety while the type assertion does not.
| isHexString(transactionMeta?.txParams?.maxFeePerGas as string) && | |
| isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas as string) | |
| isHexString(transactionMeta?.txParams?.maxFeePerGas ?? '') && | |
| isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas ?? '') |
shared/modules/transaction.utils.ts
Outdated
| const parsedData = data && parseStandardTokenTransactionData(data); | ||
| if (parsedData && parsedData.name) { |
There was a problem hiding this comment.
Using && to assert non-nullability like this is not a good pattern, since it pollutes the type signature of the assignee.
In this case parsedData gets stuck with a falsy "" type which is neither useful nor actually representative of its expected behavior.
Could we narrow parsedData down to TransactionDescription | undefined with a ternary expression? That way we can just use optional chaining for the null check.
| const parsedData = data && parseStandardTokenTransactionData(data); | |
| if (parsedData && parsedData.name) { | |
| const parsedData = data | |
| ? parseStandardTokenTransactionData(data) | |
| : undefined; | |
| if (parsedData?.name) { |
There was a problem hiding this comment.
Yeah of course ! it is definitely a better type check !
f0a044d to
0a94eac
Compare
|
Reopened here to be able to add changes |
Description
In order to contribute to ts conversion of
metamask-controller.js, we started the conversion of related files.shared/modules/transaction.utils.tsandshared/notifications/index.tswill be 2 lower level files we included here.Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist