fix usage of react state setter#26
fix usage of react state setter#26colinlohner-doordash wants to merge 1 commit intodiscord:masterfrom
Conversation
|
@faultyserver any thoughts/feedback on this PR? would love to get this into the library |
faultyserver
left a comment
There was a problem hiding this comment.
I'm curious which eslint rule you're encountering that is giving you an error. I just realized that this repo doesn't have eslint set up in it, but in my memory of working on this in a repo that did, I don't recall seeing this give an error.
Semantically, I think this is also a perfectly valid expression as-is. TypeScript's docs outline this as explicitly allowed, and I'm inclined to lean more towards typescript's behavior than a configurable eslint rule.
In JavaScript, if you call a function with more arguments than there are parameters, the extra arguments are simply ignored. TypeScript behaves the same way. Functions with fewer parameters (of the same types) can always take the place of functions with more parameters.
But, if this is a rule that is super common and an error that a lot of people would encounter, I'm not entirely against making this change.
The alternative, though, would be to make LockListener a union of types accepting either 1 or 2 arguments like:
type LockListener =
| (enabled: boolean) => unknown
| (enabled: boolean, stack: Lock[]) => unknown;though I don't actually know if that would solve the eslint issue.
(PS I would also like to add some tests in the future, though I don't really have the capacity to add them at the moment)
| export const FocusGuard = React.memo(() => { | ||
| const [active, setActive] = React.useState(false); | ||
| useLockSubscription(setActive); | ||
| useLockSubscription((newActiveState) => setActive(newActiveState)); |
There was a problem hiding this comment.
Because useLockSubscription uses the passed-in function as a dependency in the useEffect, passing an anonymous function here means the subscription will be re-installed on every render, since it gets recreated each time.
focus-layers/src/useFocusLock.tsx
Line 23 in fbffee8
It'd be nice to memoize this first if we go this route.
What?
The react useState setter, only accepts one argument, but the lock listener passes back two arguments (
active, andstack). So to fix some linting errors we are seeing, and also resolve incorrect usage of this method, I have added an anonymous function to only pass the one argument through to the setter.Additional thoughts
I think it would be really great if we could add some unit tests to this library!
P.S. Thanks for making this library, and sharing it. It is incredibly helpful for us.