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

Improvement to return type of useMouse #237

Open
pc-erin opened this issue Sep 23, 2022 · 2 comments
Open

Improvement to return type of useMouse #237

pc-erin opened this issue Sep 23, 2022 · 2 comments

Comments

@pc-erin
Copy link

pc-erin commented Sep 23, 2022

I noticed that the return type of useMouse isn't exactly reflective of the data returned.

If we made it shaped more like this:

export type MousePosition = MousePositionOver | MousePositionOut

type MousePositionOver = {
  x: number
  y: number
  pageX: number
  pageY: number
  clientX: number
  clientY: number
  screenX: number
  screenY: number
  elementWidth: number
  elementHeight: number
  isOver: true
  isDown: boolean
  isTouch: boolean
}

type MousePositionOut = {
  x: undefined
  y: undefined
  pageX: undefined
  pageY: undefined
  clientX: undefined
  clientY: undefined
  screenX: undefined
  screenY: undefined
  elementWidth: undefined
  elementHeight: undefined
  isOver: false
  isDown: false
  isTouch: boolean
}

This preserve additional type information since, if you check that mouseData.isOver is true, the inside of that conditional will see all the values as numbers and not have to check each property individually.

Also, if we switch from null to undefined for the missing values, you can use default values during destructuring:

const { x = 0, y = 0, isOver } = useMouse(ref)

The change from null to undefined would be breaking, so if you prefer we could put off merging that until a planned major version bump, but the rest should be safe.

@jaredLunde
Copy link
Owner

This sounds good to me!

@pc-erin
Copy link
Author

pc-erin commented Oct 6, 2022

Hey, so while trying to modify the hook, I noticed a few things that might be an issue.

First, on line 32, element.getBoundingClientRect is being called inside a reducer which breaks the semantics and guarantees of reducers.

Information should never be retrieved from non-deterministic sources inside a reducer. Their output should be a pure function of their input. Same for any instances of getting properties of window inside the reducer.

All of that information should be passed into the reducer via the action, not retrieved directly inside it. It will produce very strange results if anyone tries to use react time-travel debugging on that reducer, and memoization/caching will also fail to work correctly.

I can't add those more specific return types because the way the code is currently written, there is no guarantee that the pointer position (or any other information) will be present in the state when the pointer is over the element. I think this hook may need a significant amount of re-writing before we can make statements about it's return types with any certainty.

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

No branches or pull requests

2 participants