Add batch function for batching writes
#306
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* feat: add subscribeKeys function * Update size limit * Update listen-keys/index.d.ts Co-authored-by: Andrey Sitnik <andrey@sitnik.ru> * Update listen-keys/index.js Co-authored-by: Andrey Sitnik <andrey@sitnik.ru> * Formatting reversal readme --------- Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>
* Export `setByKey` function * eslint sort imports
* Prevent stale computed values * Remove atom levels The previous commit's solution to the stale atom problem also solves the diamond problem without needing special handling of atom levels. * Optimize computed dependency check using epochs * Update size-limit --------- Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>
The new code in #380 needed to be updated to reflect the new lower number of items in listenerQueue per listener.
The benchmark for `computed` on my machine increased from 3,815 ops/sec to 5,193 ops/sec
Previously, calling setKey on a $deepMap would mutate the value in-place. This can cause various problems, for example nanostores#250 and nanostores#290. Those were both solved with workarounds, however, this solves it in a better way, by not mutating the deepMap in the first place. Some advantages of this approach: * No need for structuredClone * The oldValue passed to listeners is the same object reference as the value that was previously passed. This is how immutable stores usually work, so it will align better with expectations, cause fewer issues, and allow for new use cases, like the upcoming batching PR which needs that. * Copies were already being made at each nesting level of the key being set, they just weren't being used (so maybe this was the original intent?). Either way, that means there's no new work being done, and perf will actually improve since we no longer need to call structuredClone on oldValue. As part of this change, when a key path extends past the end of an array the result will be a sparse array rather than filling it with undefined. It would have required extra code (and slower perf) to keep the old behavior, and I don't see a good reason for it. The new behavior aligns with what JS does natively, and with other stores like SolidJS. But let me know if that's a problem.
1) Stale values could be passed to listeners 2) Listeners could be called multiple times in a row at the end of a batch when an atom changed multiple times during the batch
As of the previous commit, each listener has to track oldValue anyway to avoid issues with batching, so the caller of `notify` no longer needs to provide it. Also removed `notify` as a publicly exposed function -- now that listeners are automatically skipped when the value is the same as the last call, calling `notify` with an unchanged value won't trigger the listeners, which is the reason it was made public in the first place (see https://github.com/orgs/nanostores/discussions/276). A new way to solve that problem (via a custom isEqual function) will be added in the next commit.
With the addition of `batch`, the `changedKey` argument to `listen` can't be relied on. It also improves on the old approach by not calling listeners when unrelated keys are changed via `set`. Listening to deep keys is now done via separate functions `listenKeyPaths` and `subscribeKeyPaths`, because: * It allows better performance for the more common case of top-level keys * It allows listening to top-level keys that have dots in them, (e.g. imagine a single key with the value "foo.bar") * It allows for better type definitions for the keyPaths version (not yet changed)
ai
reviewed
May 21, 2024
| @@ -210,20 +210,20 @@ Setting `undefined` will remove optional key: | |||
| $profile.setKey('email', undefined) | |||
| ``` | |||
|
|
|||
| Store’s listeners will receive third argument with changed key. | |||
| Deprecated: a store’s listeners will receive a third argument `changedKey` when | |||
There was a problem hiding this comment.
Oh, many people really loves this API. They will not like it.
Instead of deprecating let’s just explain that it will not work with batch, because there is already a case, when key doesn’t work and people is OK with it.
ai
reviewed
May 21, 2024
| gets updated. If you are fine with waiting until the end of a tick, you can | ||
| use `batched`. The only difference with `computed` is that it will wait until | ||
| the end of a tick to update itself. | ||
| ### Batching |
There was a problem hiding this comment.
Suggested change
| ### Batching | |
| ### Batching | |
ai
reviewed
May 21, 2024
| lc: 0, | ||
| listen(listener) { | ||
| listen(_listener) { |
There was a problem hiding this comment.
_var is not cool, let’s find a better name for next callback.
ai
reviewed
May 21, 2024
| @@ -92,14 +92,14 @@ | |||
| "import": { | |||
| "./index.js": "{ atom }" | |||
| }, | |||
| "limit": "265 B" | |||
| "limit": "336 B" | |||
There was a problem hiding this comment.
The price is too big for this feature.
Let’s think how we can avoid it. Maybe we can avoid isEqual?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #298. See the description of that PR for the overview. Changes in this version:
nextbranch instead ofmainnotifyfrom the public types, adds overridableisEqualfunction as alternativebatchThere are more details about each of those bullet points in the commit messages. It's probably best to start by looking at the commits individually rather than all the changes together.