Data: Implement atomic stores #26866
Conversation
|
Size Change: +985 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
add/atomic-store
branch
from
7a040c3 to
9b0860d
Compare
gziolo
added
[Package] Data
youknowriad
force-pushed
the
add/atomic-store
branch
from
9417a90 to
98f1ba6
Compare
It's a work in progress, you can ignore my ping for now. Documentation for the new package can be found here: https://github.com/WordPress/gutenberg/blob/98f1ba60a36d666c9c16d4771c28ca3ea7247730/packages/stan/README.md |
packages/stan/README.md
Outdated
|
|
||
| ```js | ||
| // No one has subscribed to the sum instance yet, its value is "null" | ||
| console.log( sumInstance.get() ); // prints null. |
There was a problem hiding this comment.
This looks wrong. If I have a React component that wants to render the sumInstance.get() value, then even the initial render should print 3, not null.
The subscription will be created only after the initial render, in a useEffect or componentDidMount callback. It can't be created anywhere else, because it's a side effect.
This is true no matter how exactly the React bindings are implemented (@wordpress/data adapter or anything else).
There was a problem hiding this comment.
This looks wrong. If I have a React component that wants to render the sumInstance.get() value, then even the initial render should print 3, not null
This is already handled properly in useSelect, we can compute the initial value before the subscription if we want. I may add a more direct way to do that with atoms: make the "resolve" a public API of the atom instance but it's very important that the atoms stay lazy. If I don't use a selector (which becomes an atom), I don't want it to keep updating its value and consuming CPU.
There was a problem hiding this comment.
This is a good remark though, and I need to think more about how to solve this in a better way: having the laziness while allowing sync resolution for react hooks.
There was a problem hiding this comment.
This is already handled properly in useSelect, we can compute the initial value before the subscription if we want.
I tested this with the following example:
const count = createAtom( 1 );
const double = createDerivedAtom( ( get ) => get( count ) * 2 );
const getDouble = ( get ) => () => get( double );
registerAtomicStore( 'standa', {
atoms: { double },
selectors: { getDouble },
} );
function UI() {
const dbl = useSelect( ( select ) => select( 'standa' ).getDouble() );
return React.createElement( 'div', null, dbl );
}
const rendered = ReactDOMServer.renderToString(
React.createElement( UI, null )
);
console.log( rendered );In the end, the double atom is really resolved even at the time of the initial render and the example correctly renders <div>2</div>.
But the reason why it is resolved is not great: the atoms in the atomic store are instantiated in the data package's own atom registry, and that registry has an onAdd callback that immediately subscribes to the new atom's changes and relays them to the globalListener. I.e., every atom has at least one subscription during its entire lifetime. That ensures that the atom is resolved in the initial render, but at the same time completely defeats the lazy optimizations.
After removing the onAdd and onDelete callbacks from the createAtomRegistry call (with SSR, we don't need any subscriptions anyway), the resolution of double is not done. The selector returns null on initial render.
There was a problem hiding this comment.
This is a good remark though, and I need to think more about how to solve this in a better way: having the laziness while allowing sync resolution for react hooks.
The selector value should be computed when calling .get(). Calling the getter is a strong-enough signal that someone is interested in the value and that it's worth computing.
There was a problem hiding this comment.
Tests are back to normal and we're gaining 10ms when typing in this PR (this is huge)
There was a problem hiding this comment.
resolving might trigger effects (async atoms) in which case, we shouldn't resolve in render phase
It's interesting that both Recoil and Jotai run the resolvers, both sync and async ones, during get. I.e., during the render phase, firing the side effects right away.
That suggests that running the resolver or any side effect during render is not bad per-se. We only need to avoid doing certain things that would make it bad:
- the resolver shouldn't do any sync state updates, like setting the
isResolvingflag and triggering asetStateby that - the resolver shouldn't allocate any resources (e.g., make subscriptions) because they won't be cleaned up in case of aborted render
- the resolver should be resilient against running twice
Looking at Apollo Client, I see they execute the query in two phases: queryData.execute() during render, and then queryData.afterExecute() in a useEffect hook. Only the latter can allocate something and be sure that it will be cleaned up.
To implement a good React binding, maybe exposing only the simple get primitive and nothing else is not enough? The current get does two tasks at once: "synchronously reading the value" and "triggering the resolution". There can easily be two lower-level primitives that do that separately.
There was a problem hiding this comment.
To implement a good React binding, maybe exposing only the simple get primitive and nothing else is not enough? The current get does two tasks at once: "synchronously reading the value" and "triggering the resolution". There can easily be two lower-level primitives that do that separately.
Yes, I agree, I think there's value in "execute" and "afterExecute" to handle things like resolution state... but I believe we can start with the current API and see later. The important thing is to ensure @wordpress/stan remains a bundled package (like icons and interface) where we're able to make breaking changes. (I'll update the building to do that)
There was a problem hiding this comment.
but I believe we can start with the current API and see later.
My perspective is that I'd like to implement something like React Query on top of the stan store. That means maintaining mainly collections rather than individual atomic values, performing CRUD operations on them, acknowledging that these operations can fail and having first-class support for error handling, having control over the cache ("invalidating resolution" in the language of @wordpress/data)...
Recoil or Jotai provide some basic support for that, but I feel I'd quickly hit a wall when trying to implement some of the more complex requirements.
There was a problem hiding this comment.
yes, that's definitely where we'll see the most complex use-cases :) (basically, how to write core-data with stan).
I believe as is right now, it will provide a lot of good for performance for things like block-editor store but handling the complexities of something like core-data is still something that need to be tested.
youknowriad
force-pushed
the
add/atomic-store
branch
from
0c6cd78 to
7fa875a
Compare
|
The API right now is a bit inconsistent.
We need to consolidate on a single naming: "read/write" or "get/set". not sure what's better? |
Why did you choose to hide the atom instances completely, allowing access only through the registry as a proxy? An alternative is splitting the registry interface into two: registry.get( atom ).read();
registry.get( atom ).write( value ); |
Because the concept of atom instances is a bit confusing IMO and users shouldn't have access to it. (implementation detail). I actually already have the "get" function you propose but I'm marking it unstable. Also, now I'm considering renaming "registry" to just "store" because it's what holds the actual state. I think it's a bit better. |
The concept still needs to be a part of the programmer's mental model, doesn't it? They need to know that |
|
Here's how I see the mental model evolving.
Note that I didn't use "atom state" here at all, I just used "value of an atom" or "state of an atom", I believe it's simpler to understand that way. Basically, for someone familiar with redux stores, the difference would be that the store holds states per atoms and not just a global state. In this mental modal, the atom is an "identifier". |
|
Regardless though, we still need to figure out if use read/write or get/set for the APIs :P |
| let mapOutput; | ||
| const [ , dispatch ] = useState( {} ); | ||
| const rerender = () => dispatch( {} ); | ||
| const isMountedAndNotUnsubscribing = useRef( true ); |
There was a problem hiding this comment.
There is a package called use-subcription, directly from the React team, that provides a useSubscription hook for subscribing to things from React components in async mode.
It solves all the gotchas and race conditions that need to be taken care of. Using it in useSelect (both in this new and the legacy version) would simplify the hook tremendously, outsourcing most of the complexity. All that remains is calling mapSelect and memoizing it properly, and handling the "async mode" where the state updates are fired through the priority queue rather than done directly.
There was a problem hiding this comment.
Oh good reminder, I actually did try that package once of useSelect but at that time, it was causing a big loss in performance, maybe with the atoms implementation, it will be better.
Co-authored-by: Greg Ziółkowski <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
There was a problem hiding this comment.
In my opinion, this proposal is already in a mergeable state. @youknowriad, you did amazing job bringing the latest development in state management to WordPress. The performance implications are the best justification that new concepts introduced are worth exploring further:
I hope to see more improvements as we try to apply the new type of store to the most popular stores.
As noted in my comments, it's a bit unfortunate that we don't have the store (keyboard shortcuts) covered with unit tests which would give 100% confidence that the transition from Redux to @wordpress/stan is bulletproof. At the same time, this code is well tested indirectly through a huge number of e2e tests so it makes me feel more comfortable.
I'd love to hear from @jsnajdr and @adamziel, how they feel about the internal implementation of @wordpress/stan (that part is more approachable to me) and in particular, about optimizations applied to @wordpress/data. I'm not as familiar with Redux, recoil, and others, but seeing the number of unit tests that existed before and were added makes me confident that is good enough to proceed.
Co-authored-by: Jon Surrell <[email protected]>
|
@sirreal I actually already renamed the file and fixed the types. |
Lines 2 to 29 in 8df048c Otherwise it needs to be built on its own, i.e. |
|
@sirreal go push fixes :) |
| } ); | ||
| } catch ( error ) { | ||
| if ( unresolved.length === 0 ) { | ||
| throw error; |
There was a problem hiding this comment.
If I read it correctly, it will silence any resolution exceptions.
There was a problem hiding this comment.
I think the entire try/catch could be avoided if get: ( atom ) => { was async and defered the responsibility of waiting for dependencies to resolver function. I'll propose a change later today.
There was a problem hiding this comment.
(or there's always this functional API idea where get: (atom) would return an observable and reduce this file to ~20 LOC)
There was a problem hiding this comment.
how so? It throws the error once again?
There was a problem hiding this comment.
Only if unresolved.length === 0, I imagine there could be a case where dependent atom's resolve() throws an error in this line: return atomState.get(); and it gets supressed since unresolved.length is no longer 0. Although the solution could be maybe as simple as moving .get() above push. I didn't actually test it yet so I apologize if that's inaccurate - I'll come up with a test case later today to verify that.
There was a problem hiding this comment.
This case is very weird :P While I agree we should have better error handling, I'm not sure we should be throwing errors because in the example above, you have a failing promise and not an exception that is thrown
There was a problem hiding this comment.
@youknowriad ah that's an unfortunate syntax, just mentally replace throwing error with Promise.reject(). That's my point exactly, registry.get and registry.subscribe() both rely on a promise but both return a value - there's no way to catch the rejection (also it doesn't bubble up).
There was a problem hiding this comment.
The sum resolver with the + operation is special because + doesn't mind if one argument is null:
42 + null === 42;
null + null === 0;The sum resolver will never throw. It will return questionable sums instead 🙂
It's better to test this with a resolver that doesn't tolerate null values. Like when the upstream values are arrays.
There was a problem hiding this comment.
@jsnajdr IMHO that should be two separate tests - sum resolver is also a valid use case and we must account for tolerance of null values.
There was a problem hiding this comment.
I couldn't come up with anything that wouldn't async/await all the things, here's my attempt: #27155
|
Noting that I'm finished with the types for now, they seem to be building correctly. I'll save any further changes for follow-up PRs. |
|
Let's try that 🎉 |
|
Important note: |
This reverts commit 4cfe6ba.
This PR is an attempt to solve some of the current flaws of the data module:
the performance factor is an important one, I'd like for our applications to scale properly when we render more and more components. This can be solved by making sure components only "subscribe" to the data they're interested in. Tools like mobx, recoil and the like solve this by relying on observables basically (atoms are just hidden observables).
Recoil and jotai also have very interesting ways to define async behavior and data which can be used as a replacement for selectors + resolvers + controls + async actions. Reducing the number of concepts would make it easier to create these kinds of stores. It's a very different way of defining stores though and a mental model that requires some getting used to in the beginning but probably simpler than what we have anyway.
This is what this PR is about. It introduces a new
@wordpress/stanpackage that serves as a low-level package to build atomic stores. To learn more, read the README of the@wordpress/stanpackage in this PR.I've refactored the keyboard-shortcuts package store as an example of how to leverage atomic stores with the data package.
Notice that this PR also makes useSelect only subscribes to the stores that are used in mapSelectToProps. This and other improvements related to atomic stores are showing a 10ms gain in terms of typing performance in this PR. This is equivalent to approximatively 25 to 30% of the time it takes to type a character in long posts.
TODO
@wordpress/stana bundled package likeiconsorinterfaceAn alternative to: #26724 and #26692.
Related: #26733
closes #26849