Skip to content

fix usage of react state setter#26

Open
colinlohner-doordash wants to merge 1 commit intodiscord:masterfrom
colinlohner-doordash:fix/fix-usage-of-state-setter
Open

fix usage of react state setter#26
colinlohner-doordash wants to merge 1 commit intodiscord:masterfrom
colinlohner-doordash:fix/fix-usage-of-state-setter

Conversation

@colinlohner-doordash
Copy link
Copy Markdown

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
Copy Markdown
Author

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

Copy link
Copy Markdown
Member

@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)

Comment thread src/useFocusLock.tsx
export const FocusGuard = React.memo(() => {
const [active, setActive] = React.useState(false);
useLockSubscription(setActive);
useLockSubscription((newActiveState) => setActive(newActiveState));
Copy link
Copy Markdown
Member

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.

2 participants