Skip to content
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

fix usage of react state setter #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

colinlohner-doordash
Copy link

What?

The react useState setter, only accepts one argument, but the lock listener passes back two arguments (active, and stack). 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.

@colinlohner-doordash
Copy link
Author

@faultyserver any thoughts/feedback on this PR? would love to get this into the library

Copy link
Collaborator

@faultyserver faultyserver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -170,7 +170,7 @@ export default useFocusLock;
*/
export const FocusGuard = React.memo(() => {
const [active, setActive] = React.useState(false);
useLockSubscription(setActive);
useLockSubscription((newActiveState) => setActive(newActiveState));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

React.useEffect(() => LOCK_STACK.subscribe(callback), [callback]);

It'd be nice to memoize this first if we go this route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants