kevinrodriguez-io / flutter_graphql_hooks

Apollo-like set of hooks to be used with flutter
MIT License
11 stars 0 forks source link

Abuse of useState hook #3

Closed kranfix closed 3 years ago

kranfix commented 3 years ago

Inside useQuery, useMutation and others, there are many useState for loading and error. These generates a double rebuild for each updating depleting performance.

The right way is to create a custom hook.

kevinrodriguez-io commented 3 years ago

@kranfix Thanks for creating this issue.

I don't understand this problem, because it can't see any instance of it happening.

This log is from a is from the useQuery hook I'm using right now as v ^1.0.2-beta:

flutter: Rerendering because of rooms:
flutter: UseQueryHookResult{result:GraphQLHookResult{loading:true, data:null, error:null}, fetcher:Closure: () => Future<Null>}
flutter: Rerendering because of rooms:
flutter: UseQueryHookResult{result:GraphQLHookResult{loading:false, data:{rooms: [{__typename: rooms, id: 64, name: High jackets}, {__typename: rooms, id: 71, name: Elementary reason}]}, error:null}, fetcher:Closure: () => Future<Null>}

This is at first render, we're getting only two render cycles: 1- One for the first loading state (render). 2- One for having loading and data set.

Which imho is the right behavior.

The reason this happens, different from react hooks for example, is because of the usage of ValueNotifier<S>. This is no abuse at all of useState and it generates only the needed rerenders. Same for useMutation.

I know where you're coming from and I know in react this is a code smell just by looking at it, but the flutter hooks library behaves a little different and provides some flutter goodies like the usage of ValueNotifier. I know you might be concerned about this because of the dependencies array of useMemoized, however, again, thanks to the implementation of ValueNotifier we don't have more render cycles than necessary, because the ValueNotifier is created once and being reused, it will be the same for the memoized method; This allows us to re-render only when needed.

@kranfix Could you provide an example of what you said happening? I'm building an app that uses this library and I haven't noticed any unnecessary re-renders yet.

kranfix commented 3 years ago

Sorry, my bad for saying rebuild instead of element.markNeedsBuild() that is called multiple times currently.  It does not call to the build method instantly, but it's preferred to call the markNeedBuild as less as possible.

On the other hand, I don't come from react.

kranfix commented 3 years ago

In addition, creating a hook class would allow to use the GraphQLResult type instead of the OperationResult. Then an extension can be used for create a isLoading getter for the QueryOperationResult.

kevinrodriguez-io commented 3 years ago

"I know where you come from" is a saying, not that you come from react, I mean that I understand what you say.

On the other hand, this sounds like a great idea @kranfix , if you want, you can draft an RFD and I'll implement it or create a PR. If you'd leave it to me I'd create a reducer and useReducer state instead of state variables, but a custom hook can be created too. I find it valuable to expose the GraphQLResult and having a getter is also a great Idea. I'll add all-contributors to this repo eventually.

kevinrodriguez-io commented 3 years ago

@kranfix Basically, the idea behind abstracting operations like query/mutation/subscription into a loading/error/data(and eventually more) is to avoid boilerplate code, similar to apollo, and we can eventually return more helpful data, like the variables that were used for example, in the same fashion as https://www.apollographql.com/docs/react/api/react/hooks/#result.

Before abandoning the current functional code, I'll try to reproduce the markNeedsBuild(); and profile all of the build cycles, because I have the gut feeling that this is already being taken care of in the way the flutter_hooks library is laid out.

kranfix commented 3 years ago

Most of the helpful data are method or getters, thats why it is preferred to create a class with all those functionalities. It will make the code more readable.

And as a contributor of flutter_hooks package, It is preferred to create a Hook class instead of composing with many other unless this composition was small and simple

kevinrodriguez-io commented 3 years ago

Most of the helpful data are method or getters, thats why it is preferred to create a class with all those functionalities. It will make the code more readable.

And as a contributor of flutter_hooks package, It is preferred to create a Hook class instead of composing with many other unless this composition was small and simple

I respect that. So, whats the idea? Turn the hooks into hook classes right ? ( hook and hookstate)

I'd like to keep the Apollo-like api to avoid boilerplates for the end user, what do you think?

kevinrodriguez-io commented 3 years ago

I had a talk with the creator of flutter hooks and this seems to be overkill, closing the thread since it's way too opinionated, please create an rfc discussion for api changes.