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

fix: useContractRead & useContractReads data instability #659

Merged
merged 4 commits into from Jul 5, 2022

Conversation

jxom
Copy link
Member

@jxom jxom commented Jul 4, 2022

Description

Fixes #648.

@changeset-bot
Copy link

changeset-bot bot commented Jul 4, 2022

🦋 Changeset detected

Latest commit: ea47b48

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wagmi Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jul 4, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
wagmi ✅ Ready (Inspect) Visit Preview Jul 5, 2022 at 0:17AM (UTC)

queryKey_,
watch,
])
useInvalidateOnBlock({ enabled: watch && !cacheOnBlock, queryKey: queryKey_ })
Copy link
Member Author

@jxom jxom Jul 4, 2022

Choose a reason for hiding this comment

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

The old implementation was refetching on provider changes (as well as block number changes), but we are already checking for provider changes in our query key (via chainId), so it was double fetching. We still want to watch on block number, so I have refactored this to "invalidate on block" if watch & !cacheOnBlock is truthy.

@matthewlilley
Copy link

matthewlilley commented Jul 4, 2022

I'm not super familair with changesets yet, I've been meaning to dig into it a bit more for our own use, but I've seen when changesets are added that pre-release versions are immediately published. I wonder if you'd be able to set that up for wagmi. Would be super useful. Example in-case you're interested in digging:

graphprotocol/graph-client#108 (comment)

@tmm
Copy link
Member

tmm commented Jul 4, 2022

Great idea! We can set up Snapshot Releases to do this. There's some work in progress that will make this even easier to set up. Looks like it's moving quickly so can prepare a PR for setting this up.

Copy link
Member

@tmm tmm left a comment

Choose a reason for hiding this comment

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

Want to move the deepEqual implementation into @wagmi/core? Seems useful to have an ethers-compatible implementation live there (e.g. for dealing with things like Result). Also, should we expose it from wagmi too - now that it's a default for isDataEqual?

Comment on lines 20 to 29
React.useEffect(() => {
if (!enabled) return
if (blockNumber === startBlock.current) return
if (!startBlock.current) {
startBlock.current = blockNumber
return
}

queryClient.invalidateQueries(queryKey)
}, [blockNumber, enabled, queryClient, queryKey])
Copy link
Member

Choose a reason for hiding this comment

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

If useBlockNumbers implementation was updated slightly, we could use the onSuccess callback instead of a separate effect here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Updated, and now the implementation is more basic 👌


import { useBlockNumber } from '../network-status'

export function useInvalidateOnBlock({
Copy link
Member

Choose a reason for hiding this comment

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

Think it's useful to have a basic test that checks the query cache between new blocks?

Not easy to do with our current testing setup, but maybe at least we should add a test.todo.

@@ -11,6 +11,8 @@ import { useProvider, useWebSocketProvider } from '../providers'
import { useChainId, useQuery } from '../utils'

type UseBlockNumberArgs = Partial<FetchBlockNumberArgs> & {
/** Function fires when a new block is created */
onBlock?: (blockNumber: number) => void
Copy link
Member

Choose a reason for hiding this comment

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

pretty awesome

@jxom jxom merged commit be76586 into main Jul 5, 2022
@jxom jxom deleted the jxom/fix-unstable-data branch July 5, 2022 21:33
@github-actions github-actions bot mentioned this pull request Jul 5, 2022
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.

[bug] useContractReads data instability
3 participants