Closed diegohaz closed 2 years ago
This makes sense because that's exactly how React.useState works. But it's ultra verbose. Also, not all states have their respective updaters.
Personally, I don't mind the verboseness, I can always extract the complexity away into my own hook. Having it work differently from React.useState
would add some magic.
I like the alternative of exposing a hook.setState
although I do wonder if it will lead to leaky abstractions. Setting some state that was never meant to be changed for example.
My suggestion would be to provide methods for all states like your tooltip example.
Now that I’m thinking more about that, I agree with you @tom-sherman! So, every initial state property should have its respective setProperty
function.
So we have to map all the missing ones and add them.
On that note, place
should be setPlacement
instead.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
I've been trying to think on a solution for this issue. I've been seeing more and more people trying to create controlled components at the same time they want to use Reakit state hooks. And I agree that sometimes you really need both.
For example, the component below will only consider the initial value passed to the visible
prop:
function Component({ visible }) {
const disclosure = useDisclosureState({ visible });
...
}
We can't update the internal visible state from outside:
const [visible, setVisible] = React.useState(false);
<Component visible={visible} />
// This button will do nothing
<button onClick={() => setVisible(!visible)} />
In order to update the internal state based on a prop, we have to use the functional version of getDerivedStateFromProps
(we could use React.useEffect
here too, but it would be a bit more expensive):
function Component({ visible }) {
const disclosure = useDisclosureState({ visible });
if (visible !== undefined && visible !== disclosure.visible) {
disclosure.setVisible(visible);
}
...
}
This is okay if you have to do it for 1 or 2 states, but it gets more and more verbose the more is the state you want to derive from props. And not to mention that not everyone knows about this approach.
But things get really worse when you introduce some kind of event prop that will react to the internal state change. For example, on a Dialog component you would want to know from outside if the internal visible
state has changed in case the user presses Escape to close the dialog.
The idiomatic way to do this is by hooking into the state actions like this:
function Component({ visible, onVisibilityChange }) {
const state = useDisclosureState({ visible });
if (visible !== undefined && visible !== state.visible) {
state.setVisible(visible);
}
const disclosure = {
...state,
setVisible(arg) {
state.setVisible(arg);
onVisibilityChange(arg);
},
hide() {
state.hide();
onVisibilityChange(false);
},
show() {
state.show();
onVisibilityChange(true);
},
toggle() {
state.toggle();
onVisibilityChange(!visible);
},
};
...
}
I've also seen other ways to do this using React.useEffect
. But the fact that, as the library author, I can't write it off the top of my head is a signal that it would be a struggle for most users.
I'm not sure yet, but I'm thinking about a new optional parameter on state hooks for derived state. The signature would be something like this:
- useModuleState(initialState = {}) => state;
+ useModuleState(initialState = {}, derivedState = {}) => state;
The derivedState
parameter would accept all properties from initialState
in addition to the setters (e.g. setVisible
, setPlacement
, setBaseId
etc.):
function Component({ visible, onVisibilityChange }) {
const disclosure = useDisclosureState({}, { visible, setVisible: onVisibilityChange });
...
}
Internally, if properties from derivedState
were present, they would be used in place of the internal state:
https://github.com/reakit/reakit/blob/6f06f3fb9d02ce7e4cce19c52abcbb07a3499009/packages/reakit/src/Disclosure/DisclosureState.ts#L85
So deriving state from props and hooking into state actions manually would be unnecessary.
cc/ @itsjonq
oOoOOoo... I really like the derived state second argument 👏 ! It feels like a seamless addition to the existing Reakit API. Also, very easy to reason about and use ❤️
Here's another proposal based on https://github.com/reakit/reakit/issues/487#issuecomment-658376872, that will be fully possible in Reakit v3: a single parameter that can receive both uncontrolled and controlled state.
To be fully clear and prevent people from thinking they can change their value between renders, all uncontrolled state should be prefixed with initial
. This would behave the same as today, but instead of visible
, it would be initialVisible
, for example.
const state = useDialogState({ initialVisible: false }); // state.visible === false
state.show(); // state.visible === true
state.hide(); // state.visible === false
state.toggle(); // state.visible === true
state.setVisible(false); // state.visible === false
state.setVisible(prevVisible => !prevVisible); // state.visible === true
Controlled state should be passed as state
and setState
. For example, this would change the current behavior of visible
.
const [visible, setVisible] = React.useState(false);
const state = useDialogState({ visible, setVisible });
state.show(); // visible === true
state.hide(); // visible === false
state.toggle(); // visible === true
state.setVisible(false); // visible === false
state.setVisible(prevVisible => !prevVisible); // visible === true
setVisible(false); // state.visible === false
Some properties don't need initial state or setters, but, unlike today, they should be controlled.
// Changing `loop` here after the first render should affect the state.
useCompositeState({ loop: true });
initial
propscontrolledState
parameter as suggested in my last comment.initial
API (eg. visible
).// @deprecated Passing `visible` as initial state to `useDialogState` is deprecated. Use `initialVisible` instead.
useDialogState({ visible: true });
// Use this instead
useDialogState({ initialVisible: true });
// Or pass controlled state to the second parameter
useDialogState({}, { visible, setVisible });
We'll not show any deprecation warning for controlled state without setters (eg. baseId
, loop
, wrap
, orientation
etc.). But we'll have to deprecate the setters for these properties when they exist. (eg. setLoop
, setWrap
, setOrientation
etc.).
controlledState
parameter with the first one.controlledState
parameter.// @deprecated The second parameter of `useDialogState` is deprecated. Pass controlled state to the first parameter instead.
useDialogState({}, { visible, setVisible });
// Use this intead
useDialogState({ visible, setVisible });
// Or this
useDialogState({ initialVisible: true });
controlledState
parameter.
🚀 Feature request
Please, don't understand this as something I decided to do. This is just a possible idea I'm leaving here to solve a common problem. There may be better ways to approach this problem.
Motivation
Right now, state hooks only accept an initial state argument, which means it can't be changed on subsequent renders:
The only way to update
placement
in the example above is to calltooltip.place()
:This makes sense because that's exactly how
React.useState
works. But it's ultra verbose. Also, not all states have their respective updaters.This makes it harder to compose state hooks and build reusable components on top of them.
Example
This would be the same as today, but passing different values to the state hooks would update its internal state.
Possible implementations
I'm gonna use
HiddenState
as an example. Currently, we useuseSealedState
to make sure values will not change: https://github.com/reakit/reakit/blob/04d1f8cbb2258fa260b02682fd3730f20deac173/packages/reakit/src/Hidden/HiddenState.ts#L81-L86Besides removing
useSealedState
, we would have to add effects to update each internal state (at least the ones that are initiated with the arguments).Implications
This works great for primitive values: strings, numbers etc. For arrays and objects, for example, this would be a nightmare. Users would have to memoize them before passing to the state hook, which would make the API even harder to use. And I bet that most of the times they wouldn't do that.
Alternatives
One alternative could be providing a straightforward way to update the values passed as initial state, for example:
Or we can just make sure to provide updaters for all states.