Conversation
mcmire
left a comment
There was a problem hiding this comment.
Thanks, this makes sense! Could you mark this as a breaking change for both safelyExecute and safelyExecuteWithTimeout in the PR description? I've also left a couple of comments and a question.
| return { | ||
| ...openSeaMetadata, | ||
| name: blockchainMetadata.name ?? openSeaMetadata?.name ?? null, | ||
| name: blockchainMetadata?.name ?? openSeaMetadata?.name ?? null, |
There was a problem hiding this comment.
@bergeron How would you describe the impact of these changes? It looks like they'd have a functional impact.
This suggests that there may have been cases where blockchainMetadata was nullish, which previously would have thrown an error on this line. But now it's no longer thrown.
Similarly, below in getNftContractInformation, the changes suggest that address and contract.name would previously be missing sometimes, but now they're always included.
Is that the case? Or are the affected scenarios really non-existent, with these changes being made just for type reasons.
There was a problem hiding this comment.
It looks like this does fix something. In the case where the request fails, this would crash on this line previously, but now it doesn't.
Too bad this isn't represented in tests either, there are no tests for this method (as it's private). And even indirect uses of it are stubbed out in the tests 🤦
Explanation
safelyExecuteandsafelyExecuteWithTimeoutare convenient utility functions, but they were returninganywhich removes type safety. This PR makes them generic so they return the type of the underlying operation.References
Changelog
@metamask/controller-utilssafelyExecuteandsafelyExecuteWithTimeoutgeneric so they preserve typesChecklist