Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analytics #1777

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Analytics #1777

wants to merge 7 commits into from

Conversation

greg-schrammel
Copy link
Contributor

@greg-schrammel greg-schrammel commented Dec 3, 2024

Fixes BX-1709 BX-1708 BX-1712
Figma link (if any):

What changed (plus any additional context for devs)

same as rainbow-me/rainbow#6287

Screen recordings / screenshots

What to test

@derHowie derHowie marked this pull request as draft December 9, 2024 17:00
@greg-schrammel greg-schrammel marked this pull request as ready for review December 10, 2024 04:42
// we are tracking 'Insufficient funds' 'Out of gas' 'No routes found' and 'No quotes found'
if (!quote || !inputAsset || !outputAsset || !inputAmount) return;

if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the point of the lastParams logic

Copy link
Contributor Author

@greg-schrammel greg-schrammel Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to not send repeated events, like if nothing changed but it triggered a refetch or something, it's the exact same code as the app

return false;
},
exact: false,
})[0];
Copy link
Member

@benisgold benisgold Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is dangerous, what if the result of getQueriesData is undefined or []?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-query should always return an array, [] is desired

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you can't get the 0 index of an empty array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's undefined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh lmao

chainId,
time,
}: PriceChartQueryKeyArgs) {
return queryClient.getQueriesData({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you pass a type parameter here like getQueriesData<QueryKeyType>() you won't need to do the type assertion on line 169

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the generic types the data, not the key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might be able to specify the key type like getQueriesData<unknown, Error, unknown, PriceChartQueryKey>()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no 😔

@DanielSinclair
Copy link
Collaborator

@derHowie Need 1 more review here when you get a chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants