-
Notifications
You must be signed in to change notification settings - Fork 47.8k
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
useConst() #32490
base: main
Are you sure you want to change the base?
useConst() #32490
Conversation
Regarding open questions for discussion or changes, I would like to outline my vision that guided me // why
useConst(constFactory: () => T) : T
// and not
useConst(initialValue: (() => T) | T) : T Allowing to pass anything other than factory function violates hook design and compromises its goals. Though internally this would cost just one check during hook mount phase, this indirectly encourages the user to create objects, to pass, during a render, creating garbage objects by that, which defeats the purpose of the hook to be performant However, there may be a case when one would want to capture a props-dependent primitive value during first render pass. It would be convenient to just pass it as an argument. But allowing to pass primitives, but not objects, already starts to compromise the hook to be simple and straightforward. Also, this case should probably be considered special, given the nature of the React Component. Moreover, if there is a need to capture more than one primitive, it is more likely that a function closure will (should) be used, to create object with several properties, rather than a multiple uses of a hook. So, allowing primitives to be passed, to satisfy only a special use case seems irrational |
Although creating closures negligible for performance impacts, it still produces garbage in case if closure is used only by function passed to hook. User should not be prohibited from using it, but the keeping performance as one of the hook's goals dictates that this should be given additional attention
const constFactory = () => ({
captured: false,
capturedValue: null
})
const Component = (props) => {
const stable = useConst(constFactory)
if (!stable.captured) {
stable.capturedValue = props.valueToCapture
stable.captured = true
}
} To increase attention to this, |
I didn't dig deep enough into it, but if I understood it right, there is no concept of hooks dispatchers in server components (yet?). Thus there is no clear separation of a first and subsequent renderings. So I used constant instance of |
I'm curious why you think this is? |
Yeah I was actually thinking about adding a compiler optimization for this case, to treat setter-less states as nonreactive. |
@sebmarkbage, @josephsavona, well, actually yes, it could be considered stable. But I just rely on But as an argument for 'should not be considered', I would say that, it is because the code may change. Today you consider it stable because In addition, regarding optimizations, I was thinking that |
This is a really important point. The way to think about effects is that they can re-run arbitrarily, not only when their dependencies change. That means the code in the effect has to be idempotent, just like render. So conceptually, effects always rerun, and the deps array is a convenient shortcut for idempotence and quickly bailing out of the effect if nothing has meaningfully changed. This gets at the major challenge with |
Agreed, But then let's also look at the problem you described, deeper, because it's interesting. The facts, in my opinion, of overcoming the challenge:
For fun, as a counter-attack to the protection of idempotency concept, I suggest you hide |
useConst()
Stable. Performant. Simple.
The missing brick of stability
Local stable storage associated with the component's lifecycle is needed quite often. Typically,
useRef
/useState
used for this. However, there are cases when this is not practicaluseState
runs dead code ifsetState
is not going to be usedstate
of auseState
should not be considered stable, even ifsetState
omitteduseRef
, inside render, results in all but first object to be garbageref.current
afterref = useRef()
to avoid garbage produces boilerplate code{ current: T }
box is unnecessaryThis is solved by using a custom hook that utilizes
useRef
and returns thecurrent
propertyThere are also a number of libraries that do this
Examples
Yup, it has to run an excessive check, and even box the value second time, just because of no access to hooks dispatchers, to separate a hook living phases. It looks like creating a simple thing by complicating a more complex tool designed for different thing.
This is acceptable, as it gives the desired result, and as the only available option. But then eslint starts to warn about the need to add references to other hooks dependencies.
Here are some of the issues related to that: #16873 #19125 #20205 #20477 #20752
Arguments for not allowing to specify in the
eslint-plugin-react-hooks
settings which custom hooks should be considered stable seem convincing.It feels like something is just missing. The absence of it pushes to create workarounds and the need of workarounds to close issues produced by that. Something that should have been there from the start and revealed along with
useRef
anduseState
. So simple, thatuseRef
anduseState
could have been inherited from, if there was such a need. And here is where one may give up and specify references. Or try to make it right