rrousselGit / riverpod

A reactive caching and data-binding framework. Riverpod makes working with asynchronous code a breeze.
https://riverpod.dev
MIT License
6.29k stars 959 forks source link

Provider Hierarchy inspector #1039

Open rrousselGit opened 2 years ago

rrousselGit commented 2 years ago

Similar to the state inspector #1033, we could have a tab in the Flutter devtool that shows the graph of providers/consumers

It could look like:

image

where the various nodes would be providers/consumers

This would be especially helpful for debugging why an "autoDispose" provider is not getting disposed

adsonpleal commented 2 years ago

Hey @rrousselGit! Do you have any updates on devtools for riverpod?

I was reading your post, looking at the code at the devtools package and where the provider package sends the debug info to devtools, and I think that I might be able to help porting the same idea to riverpod.

My idea, initially, is to have the same functionality as the current Provider implementation and later try to evolve to this graph view (which I loved the idea, BTW).

Do you have any concerns about this? Maybe you've already explored this and found some limitations due to riverpod's architecture.

I see that for Provider you've added a listener overriding the InheritedElement mount method, then I was looking at riverpod's repo and found that you are already exposing riverpod:provider_changed (#356). Did you find any blocker after this?

PS: I love how you are already using riverpod on the devtools implementation for provider haha

rrousselGit commented 2 years ago

Hello!

That's fine. That is typically what I planned to do. I'm just working on other things for now

adsonpleal commented 2 years ago

Perfect, I'll start working on it then! I'll let you know when I have something to show.

adsonpleal commented 2 years ago

hey @rrousselGit! I have a working POC here.

https://user-images.githubusercontent.com/11666470/174619354-bf21d513-4957-4ca5-b293-9684b82830d5.mp4

Just want to align the UI for this first part. I basically used the same components from Provider's implementation. The only difference is that riverpod has the concept of containers, so the left panel has to show the list of containers with their provider elements.

Also, as we only have access to the containers list, it was a bit hard to get the provider runtime type, so I had to use a eval that is a bit more complex:

    final providerElementsInstance = await eval.evalInstance(
      '['
      ' for (final element in RiverpodBinding.debugInstance.containers["$id"]?.getAllProviderElements().toList() ?? [])'
      '   {'
      '     "state": element.getState()?.stateOrNull,'
      '     "type": element.origin.runtimeType.toString(),'
      '   }'
      ']',
      isAlive: isAlive,
    );

This way, for the given container ID, we get the origin type and the state value.

WDYT about the UI and the eval approach? If we are good with them I can polish my code and open a PR.

rrousselGit commented 2 years ago

Nice job!

About your query, I'd be careful about a few things:

adsonpleal commented 2 years ago

Perfect! Maybe for the error/state we could show the Result<T> directly. The only change would be one more level on the preview panel.

https://user-images.githubusercontent.com/11666470/174632911-555954d5-f625-44b7-af4c-2391298aa247.mp4

I just need to figure out the stacktrace 🤔 , it won't show using the same preview as in provider's implementation.

image

About the name it should be pretty easy, I'll try adding it here.

Now, about the out of date state, it might be a bit tricky. I tried adding a refresh here, but the eval always change the instance IDs, so I was having a hard time keeping the selected state on the right panel. But I think it is possible to have a combination of provider type + name for the selected key, this way it should work. I'm just concerned that this might not be enough to have a strong unique key.

adsonpleal commented 2 years ago

About the provider name, what do you think of doing something like this?

image

The logic is basically:

final typeString = '${node.type}()';
final title = node.name != null ? '${node.name} - $typeString' : typeString;
rrousselGit commented 2 years ago

That's fine. :)

adsonpleal commented 2 years ago

Hey @rrousselGit, I'm wondering if this will be an issue:

getState can be out of date, such as if a provider isn't listened but one of its dependencies has changed. The UI likely needs an icon to inform users of this (and maybe offer them to force a refresh of the provider)

Won't the element be removed from its container when something like this happens? I'm asking this because, as the solution is currently implemented, it will only take the containers map value once, or every time the list changes when a 'riverpod:container_list_changed' event is triggered.

Or I'm understanding it wrong? Maybe when a Provider is not a autoDispose provider and was listened once and is not currently listened this could happen 🤔 , is that it?

rrousselGit commented 2 years ago

Indeed.

But feel free to add new events to the binding class. I'd expect events for when elements are added/updated/removed

adsonpleal commented 2 years ago

Perfect, I was trying to avoid changing the binding class, so we wouldn't need to have a min version for dev-tools to work. But I think it is better to change it, so we can avoid having a too complex eval.

All of this

    final providerElementsInstance = await eval.evalInstance(
      '['
      ' for (final element in RiverpodBinding.debugInstance.containers["$id"]?.getAllProviderElements().toList() ?? [])'
      '   {'
      '     "state": element.getState()?.stateOrNull,'
      '     "type": element.origin.runtimeType.toString(),'
      '   }'
      ']',
      isAlive: isAlive,
    );

could be moved to the binding class and we could expose a list of ContainerNode that have RiverpodNodes or something like that.

rrousselGit commented 2 years ago

I'd add the current Riverpod version to the binding too

The devtool should then check that this is a supported version.

adsonpleal commented 2 years ago

Perfect! Will do that.

adsonpleal commented 2 years ago

I'm trying to find the best place to put the refresh debug event. I found that ProviderBase has a _onListen and a _onRemoveListener methods. What do you think of putting the event there? This way we would get this case when a provider has no listeners. Also, the container instance has a refresh method, which could be used to refresh the states.

The only issue is to figure out how to call the refresh via eval, worst case we could refresh every provider in the given container.

rrousselGit commented 2 years ago

What refresh event are your talking about?

adsonpleal commented 2 years ago

Like when a provider has no listeners and its value might be outdated.

adsonpleal commented 2 years ago

So we can show this info to the user, so they can be aware that the value might not be what is being shown. And have a button so they can refresh it.

adsonpleal commented 2 years ago

Or we can just show a warning for now and ignore the refresh, this would be way simpler.

rrousselGit commented 2 years ago

I'm not following. You haven't described an event here afaik

adsonpleal commented 2 years ago

The context is this comment that you've made:

getState can be out of date, such as if a provider isn't listened but one of its dependencies has changed. The UI likely needs an icon to inform users of this (and maybe offer them to force a refresh of the provider)

What I'm trying to say is that we need to notify the devtool when a provider has its listeners changed, so we can show some indicator that its value could be outdated.

When I say refresh event I'm meaning

debugPostEvent('riverpod:container_list_changed');

Or other debug event, so we can listen there on the devtool implementation.

rrousselGit commented 2 years ago

I see

Then you can probably add the logic in mayNeedDispose/flush for respectively checking listener removal / listener addition

adsonpleal commented 2 years ago

Perfect! Will do, soon I'll open a PR here and another there on devtool so we can discuss code!

adsonpleal commented 2 years ago

https://user-images.githubusercontent.com/11666470/174911002-a28792d1-ebca-4c7c-b56e-b88002afdfea.mp4

@rrousselGit just sharing with you the final experience that I have in mind. For now I'll just add a warning to providers that have no listeners. Refreshing the provider state is out of scope, maybe if we see value on this we can add on another PR.

I still need to polish the code, when I have the PRs I'll post them here.

rrousselGit commented 2 years ago

Nice!

Note that this warning sounds a bit too generic. Providers know if they are out of date (cf flush). We should be able to not render the warning unless the provider indeed is out of date

adsonpleal commented 2 years ago

You mean this flush?

I didn't get how to know that the provider might be out of date. Is it the _mustRecomputeState flag?

rrousselGit commented 2 years ago

Correct

Also mayNeedDispose may not be the correct lifecycle after all. I don't remember all the names from memory, but there's probably something more appropriate.

We'll see when you make a PR.

adsonpleal commented 2 years ago

perfect! I'll use the _mustRecomputeState flag instead of hasListeners then!

adsonpleal commented 2 years ago

hey @rrousselGit here is the first PR https://github.com/rrousselGit/riverpod/pull/1471

This is the simple one, the other for devtools is on the way, still got to finish polishing the code.

adsonpleal commented 2 years ago

@rrousselGit just opened the devtools PR, as a draft, so we can have discussions over there.

https://github.com/flutter/devtools/pull/4210

kumamotone commented 2 years ago

@adsonpleal For now my Widget Inspector is unable to display the contents of Providers and I am developing it by outputting it to the console log. So I am looking forward to this Draft being merged so much! 💙 Any obstacles regarding it?

adsonpleal commented 2 years ago

As the devtools project is owned by flutter (google team) we depend on them reviewing this PR, so IDK the process.

@rrousselGit had some code merged there before, he might know how long the process is.

rrousselGit commented 2 years ago

The draft is unrelated to this issue in fact.

This issue is about a graph of the interactions between widgets/providers, when the draft is about inspecting the state of providers.

adsonpleal commented 2 years ago

true, we ended up discussing everything here

ebelevics commented 1 year ago

I tried to download devtools files from [adsonpleal](https://github.com/adsonpleal) and followed https://invertase.io/blog/how-to-flutter-devtool-plugin but I couldn't launch it on AndroidStudio nor Terminal via dart devtools. Probably I'm doing something wrong. I thought maybe I can replace folder from flutter sdk but couldn't find it also. May I ask guidence as I couldn't find proper way to do it?

adsonpleal commented 1 year ago

@ebelevics I didn't have time to work on this for the past month. It still need some work to be ready to be used.