-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Proposed Improvements #18
Comments
I like renaming Calling Can you elaborate more on if there is real benefit to useRef over useCallback? This was a discussion before, but there was no justification for useRef, while useCallback matched the exact documentation for use case -- memoizing a function. In this case, we are memoizing the removal of a parameter. The idea is that for dependency-less memoization, useRef avoids comparisons? Is there ever a situation where a dependency-less useMemo or useCallback would be a better choice than useRef? For readability or for performance? |
A few things:
I agree with this decision. I think it is based on assumptions; it clutters the code; and its benefit depends on the use-case. As I mentioned, I don't even use this myself.
This is not the case. Within a render cycle, once you call
As I said, this is not memoizing per-se. The idea with memoizing is that if the input doesn't change you simply return the previous computed value (and if the input changes, you re-compute; save it; return the value). In your case the input value (
Both As stated in the docs:
It is exactly the same mechanism with
Also, and although hardly relevant, when you have: const x = useMemo(()=> {...}, []); You still allocation a new
My instinct is to say that there isn't such a situation, but I may have overlooked some cases? I took it to the SO community |
There is no guarantee that two calls to This is why I'll try to stay up-to-date with the SO post. I've never tried before today, but there doesn't appear to be a "subscribe" feature. |
To be honest, I just came across the React implementation for this, which I reckon is superior to the proposed above: const [, forceUpdate] = useReducer(x => x + 1, 0); |
This was the original implementation in this package, and it was requested to be changed so as not to have to deal with max integer values in [primarily older] browsers with a cap. |
const [, forceUpdate] = useReducer(x => (x + 1) % Number.MAX_VALUE, 0); ? |
See the initial commit. It was changed to be simpler to read and more performant than addition and modulo per call, much the way ref would reduce an |
Benchmarks:
Yet the slow option takes 6.8 microseconds to run - not something that needs optimising. Another option:
|
I am fond of the fact that a library that can amount to being a single line long has nothing more to worry about than micro-optimization. It's why Are you aware of any performance test suites that apply to React components? |
Have a look at benchmark. For example usage with React see Material UI component benchmarking. |
@Izhaki another one for you: const forceUpdate = useReducer(() => Symbol())[1]; |
@evoyy Symbol has poor browser support. As much as I dislike IE, I wouldn't want to cut out its users from all dependents on this package. |
Current Code
Proposed Improvements
JS, but can easily be typed:
Rational:
First,
dispatch
unclear. Looking at the history, it is a residue from whenuseReducer
was used, but this is no longer the case.Then, from the docs (see note):
So there's no need providing it as a
useCallback
dependency.Either
useCallback
oruseMemo
with empty dependencies ([]
- ie, one-off) is identical touseRef().current
which somehow communicates the intent better - this value will not change. Also, withuseRef
, react doesn't call Object.is(old, new) each render.Also note that in the current code we have
memoizedDispatch
but becausedispatch
is stable, there's no real memoization going on -useCallback
will always return the same function.Finally, in my case, initial render could trigger dozens of
forceUpdates
so a further improvement could be (I'm not actually using it because the trade-off doesn't seem to be worth it, but others may find this useful):The text was updated successfully, but these errors were encountered: