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

feat(explorer): global transactions listener #3285

Merged
merged 16 commits into from
Oct 16, 2024
Merged

Conversation

karooolis
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Oct 14, 2024

🦋 Changeset detected

Latest commit: 36b4206

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

This PR includes changesets to release 24 packages
Name Type
@latticexyz/explorer Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/dev-tools Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/stash Patch
@latticexyz/store-indexer Patch
@latticexyz/store-sync Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-module-metadata Patch
@latticexyz/world-modules Patch
@latticexyz/world 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

@karooolis karooolis changed the title feat(explorer): transactions notifications feat(explorer): global transactions listener Oct 14, 2024
@karooolis karooolis marked this pull request as ready for review October 14, 2024 13:29
@karooolis karooolis requested a review from alvrs as a code owner October 14, 2024 13:29
@@ -25,6 +27,9 @@ const formSchema = z.object({
});

export function WorldsForm({ worlds }: { worlds: Address[] }) {
// Initialize the observer store to start fetching transactions
useStore(store);
Copy link
Member

Choose a reason for hiding this comment

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

does this cause a rerender of this component every time the store changes?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the zustand suggested approach of using a provider in next.js is for this reason 🙈 I assumed it was an SSR thing but maybe its a broad solution to this particular issue

Copy link
Contributor Author

@karooolis karooolis Oct 16, 2024

Choose a reason for hiding this comment

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

yeah, unfortunately causes re-renders, it's definitely a code smell to say the least. Using the provider does feel cleaner despite the all the boilerplate. But I think the Provider solution is specifically for SSR because I'm not sure how common of a scenario it is to initialize a store in such way that we're doing it now. Usually, you'd use one of store's getters/setters, and initialize the store implicitly when it's needed.

Perhaps a scenario where a store is initialized with props is more fitting for us https://zustand.docs.pmnd.rs/guides/initialize-state-with-props#store-creator-with-createstore but if we do that, then we're back to using providers.

Given that the current problem essentially is that the broadcast channel listener does not get started, I'm thinking what about moving the broadcast channel event listener into a hook, and initialize it somewhere higher-up the tree that way? Or alternatively, go back to the Providers solution from earlier, and include observer transactions into a common store too.

Copy link
Member

Choose a reason for hiding this comment

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

do you know if the provider would solve for this?

if so I feel like maybe the provider approach is ~fine and solves for multiple problems (SSR, global mounting/initializing the store and observer)

but curious if we can generalize the store provider boilerplate to avoid repeating the pattern for every store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need to set it up to double-check but I'd think so. Given though that the store solution works, even if somewhat hacky, I'm included to stick with that

@karooolis karooolis merged commit 4b46409 into main Oct 16, 2024
13 checks passed
@karooolis karooolis deleted the kumpis/listen-txs-global branch October 16, 2024 11:26
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.

2 participants