Rewrite useImperativeHandle to use useLayoutEffect (-35 B) #2003
Conversation
|
|
||
| if (index >= hooks._list.length) { | ||
| hooks._list.push({}); | ||
| } | ||
| return hooks._list[index]; | ||
| } | ||
|
|
||
| /** | ||
| * @param {import('./index').StateUpdater<any>} initialState | ||
| */ | ||
| export function useState(initialState) { |
There was a problem hiding this comment.
When I tested it, converting this to useReducer.bind seemed to increase the size so I left it as is for now.
| prevRaf !== options.requestAnimationFrame) { | ||
| if ( | ||
| (!component._afterPaintQueued && (component._afterPaintQueued = afterPaintEffects.push(component)) === 1) | ||
| || prevRaf !== options.requestAnimationFrame |
There was a problem hiding this comment.
Hmm - I'm wondering how common the case of swapping options.requestAnimationFrame is that we'd need this. Was there a specific case where this was troublesome? Only thing I could think of was injecting a super long rAF and then wanting to cancel.
There was a problem hiding this comment.
I don't think its very common and personally would love to save some bytes here (-24 bytes!). It was orginally added in #1851 so perhaps @robertknight could chime and let us know his thoughts? Perhaps if we could fix the original act issue from #1851 in a different way, could we perhaps remove this check here?
There was a problem hiding this comment.
I am sure that the issue captured by the tests in #1851 should stay fixed, i.e. act needs to reliably flush any renders/effects triggered during its callback regardless of whether there are renders/effects happening outside of act callbacks or not. Tests can break in very confusing ways otherwise.
The Enzyme adapter installs a debounceRendering hook to ensure that it can flush pending state updates when Enzyme asks for the current render output. It doesn't access requestAnimationFrame though, so if you can find an alternative way to keep the tests added in #1851 passing, that would be fine from my point of view.
| /** | ||
| * @param {any[]} oldArgs | ||
| * @param {any[]} newArgs | ||
| */ | ||
| function argsChanged(oldArgs, newArgs) { | ||
| return !oldArgs || newArgs.some((arg, index) => arg !== oldArgs[index]); |
There was a problem hiding this comment.
I know it's not part of this PR, but I wonder if this is smaller after gzip?
| return !oldArgs || newArgs.some((arg, index) => arg !== oldArgs[index]); | |
| if (!oldArgs || oldArgs.length!==newArgs.length) return; | |
| for (let i=newArgs.length; i--; ) if (newArgs[i]!==oldArgs[i]) return; | |
| return true; |
There was a problem hiding this comment.
No worries. All comments welcome 😄
This function returns true if the args are different so I think the return values need to be swapped like so:
| return !oldArgs || newArgs.some((arg, index) => arg !== oldArgs[index]); | |
| if (!oldArgs || oldArgs.length!==newArgs.length) return true; | |
| for (let i=newArgs.length; i--; ) if (newArgs[i]!==oldArgs[i]) return true; | |
| return; |
In my tests it adds 21 B. However, given that argsChanged is potentially run a lot in render methods, perhaps it might be worth the bytes for the perf boost? (I'm assuming a for loop is marginally faster than .some).
andrewiggins
changed the title
...since its implementation does more than a real `safeRaf` implementation. In other words, `safeRaf` invokes the callback after a rAF (or timeout) and another timeout (line 241) so its not a pure `safeRaf`. So to avoid confustion, I renamed the function to be clearer and added a comment describing its purpose
andrewiggins
changed the title
This PR brings some improvements to hooks that @JoviDeCroock and I have been playing with in other branches for a little while into master.
@JoviDeCroock I thought I'd bring these in a separate PR cuz I have a couple different branches I'd like to build on top of them. That alright?