TS wallets #17733
Merged
TS wallets #17733
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjb
commented
May 28, 2019
| @@ -25,7 +24,7 @@ const common = { | |||
|
|
|||
| const enterKeyProps = { | |||
| ...common, | |||
| view: 'key', | |||
| view: 'key' as 'key', | |||
There was a problem hiding this comment.
better way to do this? type View = 'key' | 'name'
There was a problem hiding this comment.
You can do this:
'key' as const but our eslint isn't happy with it for some reason.
also see this:
typescript-eslint/typescript-eslint#312
There was a problem hiding this comment.
typescript-eslint/typescript-eslint#390
We might have to update our eslint
cjb
commented
May 28, 2019
| prevProps.senderAccountID && | ||
| (!props.transactionID || !props.senderAccountID) && | ||
| prev.transactionID && | ||
| prev.senderAccountID && |
There was a problem hiding this comment.
Is this the best way to do this?
There was a problem hiding this comment.
You can use type guards to refine the types. For example you can do this:
diff --git a/shared/wallets/transaction-details/index.tsx b/shared/wallets/transaction-details/index.tsx
index 07dd203790..1392eabcd8 100644
--- a/shared/wallets/transaction-details/index.tsx
+++ b/shared/wallets/transaction-details/index.tsx
@@ -421,6 +421,10 @@ const TransactionDetails = (props: NotLoadingProps) => {
)
}
+function isNotLoadingProps(props: Props): props is NotLoadingProps {
+ return !props.loading
+}
+
class LoadTransactionDetails extends React.Component<Props> {
componentDidMount() {
this.props.onLoadPaymentDetail()
@@ -428,11 +432,11 @@ class LoadTransactionDetails extends React.Component<Props> {
componentDidUpdate(prevProps: Props) {
// An erased transaction ID likely means the payment was updated,
// which means details need to be retrieved again
- if (this.props.loading || prevProps.loading) {
+ if (!isNotLoadingProps(this.props) || !isNotLoadingProps(prevProps)) {
return
}
- const props = this.props as NotLoadingProps
- const prev = prevProps as NotLoadingProps
+ const props = this.props
+ const prev = prevProps
if (
(!props.transactionID || !props.senderAccountID) &&
prev.transactionID &&
as should generally be avoided since it is telling the compiler "I know better than you, trust me on this."
MarcoPolo
approved these changes
May 29, 2019
| class Qualified extends React.PureComponent<Props, State> { | ||
| state = { | ||
| rowIdxLoaded: -1, | ||
| } | ||
| _loadingTimerID: ?TimeoutID | ||
| _loadingTimerID: NodeJS.Timeout |
There was a problem hiding this comment.
this doesn't have to be null | nodejs.timeout?
| onClickResult: (username: string) => void | ||
| onShowSuggestions: () => void | ||
| onShowTracker: (username: string) => void | ||
| onScanQRCode: () => void | null |
There was a problem hiding this comment.
This is a bug in the converter, sorry. This should be (() => void) | null.
| onShowSuggestions: () => void | ||
| onRemoveProfile: () => void | ||
| onChangeRecipient: (recipient: string) => void | ||
| onScanQRCode: () => void | null |
| recipientPublicKey: string | ||
| errorMessage?: string | ||
| onChangeRecipient: (recipient: string) => void | ||
| onScanQRCode: () => void | null |
shared/wallets/transaction/index.tsx
Outdated
| onCancelPayment: ?() => void, | ||
| onCancelPaymentWaitingKey: string, | ||
| memo: string | ||
| onCancelPayment: () => void | null |
shared/wallets/transaction/index.tsx
Outdated
| sourceAsset: string, | ||
| status: Types.StatusSimplified, | ||
| statusDetail: string, | ||
| onSelectTransaction?: () => void | null |
| accountID: Types.AccountID | ||
| isDefaultWallet: boolean | ||
| keybaseUser: string | ||
| onBack: () => void | null |
|
|
||
| export type Props = { | ||
| accountIDs: Array<Types.AccountID> | ||
| getAttachmentRef: () => React.Component<any> | null |
| onChanged={(node: React.Node) => { | ||
| // $ForceType doesn't understand key will be string | ||
| onChanged={(node: React.ReactNode) => { | ||
| // @ts-ignore doesn't understand key will be string | ||
| const selectedCode: Types.CurrencyCode = node.key |
There was a problem hiding this comment.
You could also do a as string since you want to tell the compiler this will be a string
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
@keybase/react-hackers