GoogleChromeLabs / react-adaptive-hooks

Deliver experiences best suited to a user's device and network constraints
Apache License 2.0
5.11k stars 114 forks source link

Misleading #11

Closed streamich closed 4 years ago

streamich commented 4 years ago

From the title and description I understood that this library should be a React hooks collection. However, when reviewing the source code I found only the Network hook to internally use React hooks APIs, the other three hooks seem to be plain JS functions that memoize the data in module scope at boot time, hence they are not really React hooks.

My understanding of a React hook is something that internally itself uses some other React hook, which means React will schedule calls to those hooks on its hook-stack. If a JS function internally does not use any of React hooks, it is not really a hook, it can be called anywhere-anytime, and hook "rules" don't apply to it. For example, in react-use we have a rule about this in CONTRIBUTING.md.

React hooks are useful for two things: (1) store state at a specific node in React's rendering tree; (2) force a React component to re-render. For example, if you take useHardwareConcurrency hook, it reports number of CPUs on user's device. But that is not state, as number of CPUs is constant and you will never need to re-render a React component because of that data as it will never change.

I hope I'm missing something, would be happy to learn that.

hugmanrique commented 4 years ago

Although I wouldn't call the library "misleading", I also had the same impression for some hooks. Needing a hook to get a property available (I'm referring to CPU threads) in the window object seems a bit wasteful and, as discussed in a post by Kent C. Dodds useMemo can actually slow down your app.

Some hooks such as useNetworkStatus seem perfect on the other hand, since phones can switch from 4G to 3G (and viceversa) quite a lot when going through poor coverage areas.

addyosmani commented 4 years ago

Thanks for raising this. I'll be closing this issue but wanted to provide some context: in the last 24 hours we've refactored the "Hooks" in this repository to shift away from using setState as the values were relatively constant for all but the network hook and didn't actually lean on firing events for state changing at all. As you noted, signals like hardwareConcurrency are completely static. This was based on community feedback and I haven't had the chance to reframe this repo - if just network is a hook, does the name still hold? Should they just be considered non-hook utilities? etc

When we first started this project, we included hooks for a number of APIs that were similar to network and made sense to provide stateful hooks for (like the Battery Status API). Now that we've mostly landed on the current set of signals, I'm very open to feedback on how the README should be modified to be clearer. Perhaps I should rephrase the README to refer to this project as React Adaptive Loading utilities.