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(stash): clean up mutations, emit updates as a list #3376

Merged
merged 18 commits into from
Jan 6, 2025

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Nov 20, 2024

No description provided.

Copy link

changeset-bot bot commented Nov 20, 2024

🦋 Changeset detected

Latest commit: e081b49

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

This PR includes changesets to release 26 packages
Name Type
@latticexyz/stash Patch
@latticexyz/store-sync Patch
@latticexyz/cli Patch
@latticexyz/dev-tools Patch
@latticexyz/explorer Patch
@latticexyz/store-indexer Patch
@latticexyz/world-module-erc20 Patch
@latticexyz/world-modules Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud 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/store-consumer Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-module-metadata 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

@alvrs alvrs force-pushed the alvrs/stash-improvements branch from aa0cf04 to 9d190f8 Compare November 26, 2024 10:52
@alvrs alvrs force-pushed the alvrs/stash-improvements branch from 34a8ee5 to c670967 Compare December 6, 2024 14:55
@alvrs alvrs marked this pull request as ready for review January 3, 2025 11:51
@alvrs alvrs requested a review from holic as a code owner January 3, 2025 11:51
Comment on lines +69 to +71
queueMicrotask(() => {
notifySubscribers(stash);
});
Copy link
Member

@holic holic Jan 3, 2025

Choose a reason for hiding this comment

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

in my entrykit branch, I had removed queuing because it caused something in the live records sync to fail and fallback to the slower sync methods and was unclear why

Suggested change
queueMicrotask(() => {
notifySubscribers(stash);
});
// TODO: wrap in queueMicrotask here once we figure out why it affects sync fallback
notifySubscribers(stash);

do you know if this is an issue here?

Copy link
Member

Choose a reason for hiding this comment

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

tested locally with a fresh template and everything seems fine

making a mental note to come back to this if we see weird issues with syncing, esp large chunks of data

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for converting to an array of state changes 🙏

still unclear if queueMicrotask is safe to do here and wondering if we tested this against a real app, or if we should remove for now

@holic holic changed the title feat(stash): various improvements feat(stash): clean up mutations, emit updates as a list Jan 6, 2025
@holic holic merged commit 96f1473 into main Jan 6, 2025
14 checks passed
@holic holic deleted the alvrs/stash-improvements branch January 6, 2025 09:19
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