[internal] Refactor the store #3384
Conversation
romgrk
added
type: enhancement
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
romgrk
changed the title
romgrk
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
| * Do not modify properties in state directly. Instead, use the provided methods to ensure proper state management and listener notification. | ||
| */ | ||
| public state: State; | ||
| state: State; |
There was a problem hiding this comment.
I want this file to be a copy-paste of the one in X, I've sync'ed the two and removed the public modifiers (which are the default) to match the convention with classes in the rest of the codebase.
There was a problem hiding this comment.
Why doesn't X import this one?
There was a problem hiding this comment.
By "this one", do you mean why isn't X importing this file directly?
It's because if we have Store tweaks to do, managing releases with BUI is too painful. I tried to do a utils-only release one time, but some people were opposed to it. So anyway, because we can't easily do tweaks on BUI utils, we have them duplicated in the X codebase at multiple places. Yay.
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
added
the
internal
github-actions
bot
removed
the
PR: out-of-date
| * | ||
| * @param key Key of the selector to use. | ||
| */ | ||
| public useState = ((key: keyof Selectors, a1?: unknown, a2?: unknown, a3?: unknown) => { |
There was a problem hiding this comment.
I've avoided the method = () => { ... } syntax when possible, because that's +1 memory allocation for each instance, and with stores being widespread it make sense to keep them lean.
There was a problem hiding this comment.
This change complexifies a bit the open-by-default feature of menu/tooltips, but it keeps the Store simple, and I'd rather have the core of the codebase be simple.
We could make the controlled props more ergonomic, I also thought about joining the two fields in some standardized way, e.g. call them open and open:controlled-prop, then we could derive selectors & effects automatically, but I've kept this simple for now.
| * Do not modify properties in state directly. Instead, use the provided methods to ensure proper state management and listener notification. | ||
| */ | ||
| public state: State; | ||
| state: State; |
There was a problem hiding this comment.
Why doesn't X import this one?
| this.setState(newState); | ||
| } | ||
|
|
||
| use<F extends (...args: any) => any>(selector: F, ...args: SelectorArgs<F>): ReturnType<F>; |
There was a problem hiding this comment.
Wasn't Store supposed to be library-agnostic with React-specific methods in ReactStore?
There was a problem hiding this comment.
I did say that but it's not a good split. We're only doing React, it doesn't make sense to consider anything else, and some of these methods are too ergonomic to skip them. What I wanted to avoid was mainly the overhead from BUI features like the controlled values, which was the main reason to not merge the store classes in a single one. Now that I've removed controlled values, I'm considering merging everything, but I'm still hesitating a bit due to the .selectors logic — the .context logic is pretty useful to have for everyone though.
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Rom Grk <[email protected]>
4 participants
Simplify controlled values and small changes to match the Store in X.