Add ContainerNode and RiverpodNode to devtool #1471

adsonpleal · 2022-06-22T20:30:54Z

This PR adds support for Flutter DevTools by improving the already present file devtool.dart.

With the changes present here we can more easily get the data needed to present the provider state on Flutter DevTools.

For more information check the discussion here.

The UI that these changes enable is the following:

final.experience.mp4

Soon I will open a PR on devtools with the addition of RiverpodScreen.

rrousselGit · 2022-06-22T21:52:18Z

👋
Could you add tests for all of this? 🙏

adsonpleal · 2022-06-22T22:48:45Z

👋 Could you add tests for all of this? 🙏

yes! Will do!

adsonpleal · 2022-06-23T17:46:23Z

packages/riverpod/test/devtool_test.dart

+}
+
+extension on ContainerNode {
+  bool isEqualTo(ContainerNode other) {


we could use freezed or equatable on the actual classes, but I noticed that riverpod package doesn't depend on these packages.

rrousselGit · 2022-07-05T09:33:02Z

packages/riverpod/lib/src/devtool.dart

@@ -1,7 +1,50 @@
-// ignore_for_file: public_member_api_docs
+// ignore_for_file: public_member_api_docs, invalid_use_of_visible_for_testing_member


What is this change for?

this is related to the other question that you made #1471 (comment) . I can totally remove it if I make it a part of provider_base.dart.

rrousselGit · 2022-07-05T09:34:30Z

packages/riverpod/lib/src/devtool.dart

@@ -70,13 +117,26 @@ class RiverpodBinding {
    return binding!;
  }

+
+  bool get supportsDevTool => true;


What is the purpose of this getter?

This was just to show or not the Riverpod tab on devtools. But I think I might have a better way of doing that. We could just use a try-catch instead. You can see the implementation here https://github.com/flutter/devtools/pull/4210/files#diff-90638f67ff3222b19bb967f40f76a108841c7a1b6917d8f853d8a04c55d0bb9eR9 .

What about making this the package version?
We'll want to expose the version anyway to know if a feature newly added devtool features are supported or not in the future.

This would of course need a unit test to check that the version is in sync with the pubspec.yaml

yes, I tried adding the version, but couldn't find a consistent way of getting if from the pubspec. But now that you mentioned the test we could just have a test that reads the pubspec and compare the versions. 🤔

Is this OK for you? We will need to also add some documentation for it, I suppose.

I was thinking here, instead of using the actual package version, why not create a "virtual" versioning for devtools support? Like we could call something like "devtoolsSupportVersion" and start it at 1, then we just increment the number every time we add a new feature. This way we don't need to always change the version when riverpod version changes, we would only need to change when we change the devtool implementation.

WDYT?

rrousselGit · 2022-07-05T09:35:08Z

packages/riverpod/lib/src/devtool.dart

+  final String id;
+  final String containerId;
+  final String? name;
+  final Result<dynamic>? state;


Suggested change

final Result<dynamic>? state;

final Result<Object?>? state;

Perfect, I'll make the change.

I can't change it to Object , the element.getState() returns a dynamic, since it is a generic.

rrousselGit · 2022-07-05T09:37:00Z

packages/riverpod/lib/src/framework/provider_base.dart

@@ -202,6 +202,8 @@ class _ProviderListener<State> implements ProviderSubscription<State> {
  State read() => listenedElement.readSelf();
 }

+var _elementDebugNextId = 0;


Move this as a static variable of ProviderElementBase

Done, just wondering if there is a reason other than keeping the consistency. In the end won't it be the same?

packages/riverpod/lib/src/framework/provider_base.dart

rrousselGit · 2022-07-05T09:38:57Z

packages/riverpod/lib/src/framework/provider_base.dart

+  ///
+  /// Should not be used, only meant for debug purposes
+  @visibleForTesting
+  bool get dependencyMayHaveChanged => _dependencyMayHaveChanged;


I don't want this to be visible. If necessary, make the devtool file in the same part as this file

I'll try making it a part, I tried going with part at first but had some issues. I'll try again and let you know.

I've removed the public getter.

rrousselGit · 2022-07-05T09:44:05Z

packages/riverpod/lib/src/framework/provider_base.dart

+  /// and [_onRemoveListener]
+  void _notifyListenersChanged() {
+    assert(() {
+      RiverpodBinding.debugInstance.providerChanged(


Why do we notify that a provider has changed when we add/remove listeners?

to be able to show the alert saying that the provider might be outdated. Like refreshing the list when this event happens.

We could just remove it, it would make the implementation smaller on both sides, but the displayed data would be less realtime.

Maybe adding a "refresh all containers" (I mean only on the devtools side, not refreshing the providers states) button would be better, this way we could refresh the devtools data. WDYT?

But why "providerChanged"? I would expect a new event for this.
We can create as many events as we want. No need to reuse the same one for multiple things

oh! Got it! Well, in the devtools implementation they would do the same thing, which is to refresh the provider state. That is why I used the same event, but I could change it to something like 'riverpod:provider_listeners_changed' and listen to both events there on devtools.

rrousselGit · 2022-07-05T09:44:51Z

packages/riverpod/test/devtool_test.dart

+  });
+
+  test('should add container to containers map', () {
+    final firstContainer = ProviderContainer();


use the createContainer utility instead of manually creating ProviderContainers

cool! I'll change that.

rrousselGit · 2022-07-05T09:45:46Z

packages/riverpod/test/devtool_test.dart

+
+    expect(
+      containerNodes,
+      predicate(


What is the purpose of the predicate here?

well, this is a creative solution to avoid adding Equatable or Freezed to the package.

As I can't compare two objects directly I created the extension functions to make the fields check without the need of changing production code. It is not the prettiest code, I know. Do you have any suggestions?

Feel free to override ==/hashCode for those classes.

perfect! This will be way cleaner hehe

added here 3fe5095

rrousselGit · 2022-08-04T13:34:39Z

I haven't forgotten about this. I'll continue the work on my own during August.

adsonpleal · 2022-08-04T18:17:20Z

I haven't forgotten about this. I'll continue the work on my own during August.

cool, so you mean you'll continue this work? I can still work on it if you want, I was just waiting for you comments.

Let me know if you need my help.

Cheers!

rrousselGit · 2022-08-04T21:51:14Z

It'd be great if you could add tests on the devtool PR (not this one, the flutter/devtool repo)

adsonpleal · 2022-08-08T12:47:29Z

It'd be great if you could add tests on the devtool PR (not this one, the flutter/devtool repo)

For sure! I was just waiting to have this API ready to go. I'll add the tests this week.

rrousselGit · 2022-08-08T13:23:58Z

Don't worry about this api. I don't mind much what it looks like, since it's not public.

What matters is that everything is tested

…to-devtool

adsonpleal · 2022-08-12T19:51:47Z

@rrousselGit this PR should be ready to be merged, we need to merge it first and create a release since devtools depends on this.

rrousselGit · 2022-08-12T20:14:02Z

Don't worry about this.

For reference, I don't plan on merging this until the devtool PR is ready to merge. These two PRs are intertined. A problem in one would require a change in the other, so they likely need to be merged at the same time.

adsonpleal · 2022-08-12T20:15:16Z

Makes sense!

rrousselGit · 2022-08-13T12:02:38Z

This is looking great!

A small note about the sidebar listing providers:

I believe that if the provider specifies a name, showing its type is redundant. Especially since we see its type once we select the provider (in the title of the right part)
I think it'd be great if providers coming from a family could list the argument

More specifically, I could see the rendering of family involve two bits.

Assuming we have:

final myProvider = Provider.family<String, int>((ref, id) => 'Hello $id');

...

ref.watch(myProvider(42));

Then I think the rendering could be:


 Containers.       |.   Exposed value
----------------------------------------
myProvider(42)     |   family arg:
                   |   42
                   |. -------- 
                   |. exposed value:
                   |. 'Hello 42'

Also, for the sake of readability, I think that if there's only one root ProviderContainer, we don't need to show it in the list. We could show the root provider directly.

This would remove one level of indentation on the sidebar

If there are multiple roots, we'd need to keep the rendering as it is. But that's fairly rare I think

adsonpleal · 2022-08-15T12:33:46Z

really great insights! I'll work on them! They should be easy to implement, I'll work on an UI and share with you soon.

adsonpleal · 2022-08-15T14:16:49Z

Kapture.2022-08-15.at.11.02.34.mp4

I've made some modifications, WDYT?

What I've changed:

1 - Don't show the container label if there is only one container.
2 - Changed the providers list header to show Providers if there is only one container, otherwise it shows Containers.
3 - Changed how the provider name is rendered, now if the provider is a family provider it also show the argument. Another changes was to remove the type if the provider is named.
4 - Also show the selected provider family argument on the right panel, only if there is one.

rrousselGit · 2022-08-15T14:22:00Z

Nice! That looks great!

Quick note, what is that "StringValue"? Is it a custom class you made as an example? Or is that the object vm_service gives you?
If that's the latter, I'd expect to see the raw string directly.

Also, we'd need to make sure that there's a text wrap if the family arg toString is too long :)

rrousselGit · 2022-08-15T14:22:45Z

Acutally, the video showcases that. Ignore me!

That's awesome, nice work!

rrousselGit · 2022-08-15T14:24:42Z

Quick note: Family args shouldn't be editable. Is that the case here?

adsonpleal · 2022-08-15T14:48:13Z

About the family not being editable:

They are, since I'm using the same viewer as the state. I just tested here and you can just edit a value if it is not final 😞.

I was looking at the instance_viewer that you've made for provider (which is the same that I'm using) and there is no option to disable editing. I could add a flag to the InstanceViewer widget like disableEditing, which defaults to false, this way we won't touch other provider implementations.

WDYT?

adsonpleal · 2022-08-15T14:49:00Z

And about text wrap:

just added a way to wrap if there is no room for the name.

rrousselGit · 2022-08-15T15:58:03Z

Sorry I used the wrong word. Rather than text wrap, I think clipping it is better. As in doing myVeryLongProvid...

The sidebar can be expanded anyway, and the provider can be selected to see it in full.
I think it's more important to see the full list

Especcially considering objects could override toString and end-up with a very long provider name.

rrousselGit · 2022-08-15T15:58:41Z

I was looking at the instance_viewer that you've made for provider (which is the same that I'm using) and there is no option to disable editing. I could add a flag to the InstanceViewer widget like disableEditing, which defaults to false, this way we won't touch other provider implementations.

WDYT?

SGTM

adsonpleal · 2022-08-15T18:30:20Z

changed

rrousselGit · 2022-08-15T18:34:57Z

Epic

A nit point would be to rename 'container" into providerContainer to be more specific. But otherwise LGTM

adsonpleal · 2022-08-15T18:41:57Z

Epic

A nit point would be to rename 'container" into providerContainer to be more specific. But otherwise LGTM

You mean written like ProviderContainer, just like the class?

adsonpleal · 2022-08-15T18:43:14Z

And what about the header? Should we keep Containers or change to ProviderContainers ?

rrousselGit · 2022-08-15T19:05:13Z

You mean written like ProviderContainer, just like the class?

Yes

And what about the header? Should we keep Containers or change to ProviderContainers ?

The header too.

adsonpleal · 2022-08-15T19:07:00Z

Done, now I just need to work on the editable flag for InstanceViewer.

rrousselGit · 2022-08-15T19:16:52Z

Perfect. Awesome work

adsonpleal · 2022-08-15T19:52:36Z

done flutter/devtools@392f6b9, let me know if you think anything can be improved.

rrousselGit · 2022-08-21T21:32:43Z

@adsonpleal do you have a social media or a name that I could share?

I'd like you publicly thank you for your work. 😊

adsonpleal · 2022-08-22T21:42:35Z

I'm one of those weird ones that don't use social media haha. I have a linkedin profile, though. I think you could use it hehe.

…e-to-devtool

adsonpleal · 2022-11-29T14:18:02Z

@rrousselGit I still didn't have time to update to the new riverpod 2.0 on devtools. It shouldn't be that hard, but I'm full with my work here and couldn't focus on that. Do you know how likely is to get the devtools PR approved? I'm wondering about the changes of that PR getting merged. Do you think is worth investing time making it ready for review?

I know that they are having discussions about creating some kind of api to create extensions to devtools. But is this actually happening on a near future?

Another alternative would be to have the riverpod inspector as a stand alone program, just like isar does with their isar inspector. With this we would have full control of the inspector, without the need of waiting for devtools approval. I know that having the tool inside devtools is way better, as de developer experience would be fully integrated with the flutter tooling. But maybe is worth going with this route, since we could have way faster releases on the riverpod devtools.

WDYT?

adsonpleal · 2023-03-02T15:53:54Z

Hey @rrousselGit since the devtools PR was closed and this code is already too outdated I'll be closing this PR.

What do you think about the custom dev-tool idea?

rrousselGit · 2023-04-13T08:32:20Z

@adsonpleal Sorry I missed your reply.

Yeah I'm considering a custom devtool too. Maybe an extension in VScode

adsonpleal · 2023-04-13T13:53:39Z

@adsonpleal Sorry I missed your reply.

Yeah I'm considering a custom devtool too. Maybe an extension in VScode

@rrousselGit Let me know if you need some help. I did some explorations here with the Isar idea (stand alone solution), but a VScode extension seems better. Or maybe even both, we could probably render the flutter web version inside a Webview in VSCode and have a stand alone web version as well.

We can explore https://github.com/Dart-Code/Dart-Code the official dart extension, they probably render the flutter stuff in VScode.

Add ContainerNode and RiverpodNode to devtool

f1a50b2

adsonpleal mentioned this pull request Jun 22, 2022

Provider Hierarchy inspector #1039

Open

add tests

5ccdcfd

adsonpleal commented Jun 23, 2022

View reviewed changes

rrousselGit reviewed Jul 5, 2022

View reviewed changes

packages/riverpod/lib/src/framework/provider_base.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Jul 5, 2022

View reviewed changes

adsonpleal added 2 commits July 5, 2022 18:49

PR requested changes

9200103

Add == to RiverpodNode and ContainerNode

3fe5095

adsonpleal requested a review from rrousselGit July 5, 2022 22:21

adsonpleal mentioned this pull request Aug 12, 2022

Add riverpod tool flutter/devtools#4210

Closed

8 tasks

adsonpleal added 3 commits August 12, 2022 16:29

add binding version

3f0f62c

Merge remote-tracking branch 'origin/master' into add-container-node-…

c754540

…to-devtool

Merge conflicts

32584b2

Add family argument

3fc811f

adsonpleal added 2 commits August 22, 2022 18:52

Merge remote-tracking branch 'upstream/master' into add-container-nod…

ff5c743

…e-to-devtool

lint fix

956403b

adsonpleal closed this Mar 2, 2023

		@@ -1,7 +1,50 @@
		// ignore_for_file: public_member_api_docs
		// ignore_for_file: public_member_api_docs, invalid_use_of_visible_for_testing_member

Add ContainerNode and RiverpodNode to devtool #1471

Add ContainerNode and RiverpodNode to devtool #1471

Conversation

adsonpleal commented Jun 22, 2022

rrousselGit commented Jun 22, 2022

adsonpleal commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adsonpleal Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrousselGit commented Aug 4, 2022

adsonpleal commented Aug 4, 2022

rrousselGit commented Aug 4, 2022

adsonpleal commented Aug 8, 2022

rrousselGit commented Aug 8, 2022

adsonpleal commented Aug 12, 2022

rrousselGit commented Aug 12, 2022

adsonpleal commented Aug 12, 2022

rrousselGit commented Aug 13, 2022 • edited Loading

adsonpleal commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 15, 2022

adsonpleal commented Aug 15, 2022

rrousselGit commented Aug 21, 2022

adsonpleal commented Aug 22, 2022

adsonpleal commented Nov 29, 2022

adsonpleal commented Mar 2, 2023

rrousselGit commented Apr 13, 2023

adsonpleal commented Apr 13, 2023

adsonpleal Jul 5, 2022 •

edited

Loading

rrousselGit commented Aug 13, 2022 •

edited

Loading