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

Add batch function for batching writes #298

Closed
wants to merge 1 commit into from

Conversation

russelldavis
Copy link

The existing batched function has a few problems:

  • It runs callbacks using setTimeout, which is less efficient and can occasionally be problematic if you're expecting events to fire in a certain order.
  • It requires the consumer of the state to opt in to using it at each call site. This requires extra mental overhead and is susceptible to mistakes (forgetting to use batched instead of computed).
  • Even if you always use batched, it still allows invalid/stale intermediate values to be observed via subscribe. For example, if I want to log both $foo and $bar, but only when $foo changes (not when $bar changes), my subscription to $foo could see the invalid/stale intermediate value of $bar when they change at the same time. To fix this, I'd have to do a setTimeout hack in my callback like batched does.
  • If I'm creating a library that exposes a bunch of atoms, I want to be able to batch the library's writes to those atoms so the users of my library don't have to think about any of the above.

For the reasons above, all state management libraries I know of that do manual batching do it on event production rather than event consumption. Examples:

This PR brings that same ability to nanostores with a new batch function. For all writes that happen inside the batch callback, notifications won't be sent until the callback completes. Example:

const $sortBy = atom('id')
const $categoryId = atom('')

export function resetFilters () {
  batch(() => {
    $sortBy.set('date')
    $categoryId.set('1')
  })
}

If you batch your writes using batch, batched is no longer necessary, so it might make sense to deprecate batched and encourage batch instead. Let me know what you think, I can update the PR with docs.

The implementation is simple and minimal because there was already a listenerQueue that was doing implicit batching for writes happening inside listener callbacks. This only adds 22 bytes to the min bundle size and 18 bytes to the max size. If batched eventually gets removed, it will probably be a net reduction in bytes.

@ai
Copy link
Member

ai commented May 2, 2024

I like the idea.

But what do you think of batched analog of computed? Should we somehow integrate it to here?

export let batched = (stores, fn) => computedStore(stores, fn, true)

cc @dkzlv

@russelldavis
Copy link
Author

@ai not sure if I'm misunderstanding your question, but the PR description has a bunch of details on why this is an improvement over the existing batched function. The issues can't really be addressed using the existing batched pattern, because to solve them the batching needs to happen where the state gets updated, not where the updates get consumed.

@ai
Copy link
Member

ai commented May 3, 2024

Sorry, I was too tired on initial review. Thanks for explaining it shorter in next comment.

Seems good for me. We can deprecate batched in next release.

Let’s wait for @dkzlv point of view.

@ai ai changed the base branch from main to next May 6, 2024 09:16
@ai
Copy link
Member

ai commented May 6, 2024

We need to rebase this PR to next too

@dkzlv
Copy link
Member

dkzlv commented May 6, 2024

@ai @russelldavis Yeah, we were talking about implementing transactions for a while now. Tbh, the implementation is very good, tests are as well, I don't see a reason not to merge this. Good job, thanks!

If you batch your writes using batch, batched is no longer necessary

I'm not sure if I agree or disagree.
I introduced batched as a very simple hacky way to make this thing work in nanostores/query (our very own swr/tanstack-query).
When you create a fetcher store, you provide it with atoms to set up the reactivity for fetches:

const $post = createFetcherStore([$categoryId, $postId]);

Initially I turned this array of dependencies into a single computed store, but I quickly found out that I need some transactions for it to work, because otherwise nanoquery would fire unnecessary fetches for incorrect state (say, new category but old post).
That's where batched shines.

Now, 2 things to consider here:

  1. I really want this to be an implementation detail of nanoquery. I don't want to force the users to use batch with all key dependencies, because they will inevitably miss this at some point, and my lib will do strange things. I feel like a library should protect a developer from doing the wrong thing.
  2. I feel like not all set calls can predictably be batched—at least, as I understand it. For example, if I depend on a library that doesn't use batch but sets multiple stores on which I depend, I cannot control that.

So it's exactly what you point out that batch and batched just cover different cases: consuming and setting state.

@ai
Copy link
Member

ai commented May 6, 2024

We can move batched to nanostores-batched repo (with duplication of computed code). So you will be able to use it, but we will improve Nano Stores code by:

  1. Avoid confusion between batch and batched
  2. Avoid having batched code in all users JS bundle (who even don’t use batched). We will save 22 bytes.

@russelldavis
Copy link
Author

russelldavis commented May 8, 2024

@dkzlv @ai thanks for the review! Totally agreed, batched still makes sense for cases where you don't control the the writes.

Given that, and the reasons below, I'm actually having second thoughts about moving that to a separate package:

  • The difference in bundle size is pretty small
  • We will have to explain the difference between them anyway
  • Keeping batched in core avoids a breaking change
  • Duplicating the code to another package adds a burden of keeping them in sync when changes are made

Let me know what you guys think.

We need to rebase this PR to next too

I have a branch where I've done this. I'm working on some perf optimizations and fixing an issue that this original PR didn't address: making sure multiple writes to the same atom inside a batch only calls the listeners once. I'll submit a new PR when it's ready.

@russelldavis
Copy link
Author

Superseded by #306

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.

None yet

3 participants