Use custom hook to create callback constants #933
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d77bbbf:
|
erikras
force-pushed
the
callbacks-as-constants
branch
from
9d2de52 to
c34f8ee
Compare
erikras
force-pushed
the
callbacks-as-constants
branch
from
c34f8ee to
af1c2cf
Compare
src/useField.js
Outdated
| state.change(format(fieldState.value, state.name)) | ||
| } | ||
| }, | ||
| [form, format, formatOnBlur, state] |
There was a problem hiding this comment.
Provided in the same order as line 172.
src/useField.js
Outdated
| : event | ||
| state.change(parse(value, name)) | ||
| }, | ||
| [component, name, parse, state, type, _value] |
There was a problem hiding this comment.
Provided in the same order as line 191.
src/useConstantCallback.js
Outdated
| const refs = deps.map(React.useRef) | ||
| // update refs on each additional render | ||
| deps.forEach((dep, index) => (refs[index].current = dep)) |
There was a problem hiding this comment.
Doesn't refs get created at each render, this forEach line is not needed 🤔
There was a problem hiding this comment.
No they don't. useRef is similar to useState -- whatever is passed as it's first param only initialises the state/ref in the very first render.
There was a problem hiding this comment.
@erikras My understanding is that you shouldn't update refs in render function as it would break with Suspense. See Dan's comment on this issue: facebook/react#21191 (comment)
There was a problem hiding this comment.
Understood, even if the deps.map is called each time, it will still get a new array but with first deps
There was a problem hiding this comment.
I think he probably means why not a single ref containing the deps array, instead of a ref for each dep. Then you don't need the forEach line, or the length check.
There was a problem hiding this comment.
I think he probably means why not a single ref containing the deps array, instead of a ref for each dep. Then you don't need the forEach line, or the length check.
I wrote this yesterday, and had this thought upon waking this morning. Why not save the whole array? 👍
src/useConstantCallback.js
Outdated
| */ | ||
| export default function useConstantCallback(callback, deps) { | ||
| // initialize refs on first render | ||
| const refs = deps.map(React.useRef) |
There was a problem hiding this comment.
It might be useful to add this line as a safety net for sanity
if (deps.length !== refs.length) {
console.warn("Length of `deps` shouldn't change between renders")
}
src/useConstantCallback.js
Outdated
| * Creates a callback, with dependencies, that will be | ||
| * instance === for the lifetime of the component. | ||
| */ | ||
| export default function useConstantCallback(callback, deps) { |
There was a problem hiding this comment.
Would something like the following help? It will always return the same instance for === purposes, but will call
the latest function passed in. No need to care about callback dependencies at all:
export function useWrappedCallback(callback) {
const ref = React.useRef(callback);
React.useEffect(() => { ref.current = callback; });
return React.useCallback((...args) => ref.current.apply(null, args), []);
}
There was a problem hiding this comment.
Ooh!! I like this a lot!
There was a problem hiding this comment.
This is epic. I had to stare at it for some time tho. So basically we always return the same identity which is inline with earlier version. BUT we work with inner function which is passed from render which is up to date on every render, thus no need for deps at all indeed and also this scales perfectly, i.e. over time when new vars are added or removed we have to change NOTHING! If I knew about this smart pattern I'd use this instead in previous PR. So I am all for it. (cc: @erikras )
There was a problem hiding this comment.
Updated PR with your implementation. MUCH nicer!
erikras
force-pushed
the
callbacks-as-constants
branch
from
2782fd2 to
d77bbbf
Compare
|
Published in |
Further abstraction around #931.