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

Add tab for inspecting the state of package:provider #2851

Merged
merged 46 commits into from
Apr 29, 2021

Conversation

rrousselGit
Copy link
Contributor

No description provided.

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how much functionality are you using from riverpod? Unless it is extensive it could make sense to avoid the dependency for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used extensively
With it, the implementation of the instance viewer is fully declarative, and it takes care of the process of canceling pending evaluations whenever possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Can you point to the pattern that ensures evaluations are cancelled?

Copy link
Contributor Author

@rrousselGit rrousselGit Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the "autoDispose" functionality on providers (which package:provider sadly cannot support)
https://riverpod.dev/docs/concepts/modifiers/auto_dispose

It keeps track of the number of listeners on the associated provider (be it widgets or other providers (such as when chaining providers)). And when all listeners are removed, the provider state will automatically be destroyed.

Then providers can add a dispose method to cancel pending requests. That's the "Disposable" object which all providers use:

AutoDisposeProvider((ref) {
  final isAlive = Disposable();
  // tell Riverpod to dispose the "Disposable" object when the provider is disposed
  ref.onDispose(isAlive.dispose);
  
  
  // when the provider is destroyed, `isAlive` will be disposed, canceling the pending requests.
  await eval.eval('...', isAlive: isAlive);
})

This combines nicely with the collapse functionality too.
All object properties are associated with a unique provider. So when collapsing a property, its listener is removed, its state destroyed and pending requests canceled.

Copy link
Contributor Author

@rrousselGit rrousselGit Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This architecture also automatically deals with loading and error handling.

It relies on the fact that using await throws when the awaited future fails, purposefully letting the exception bubble. Then the provider will catch the exception, and use a union-type to force the UI to deal with loading/errors (or the application otherwise will not compile)

That's the when(loading: ..., error: ..., data: ...) you can see inside widgets.

);
}

class _SplitBorder extends StatelessWidget {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you attach an updated screenshot to the CL?
I'm interested in seeing how this widget looks as part of the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2021-04-01 at 03 43 03

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand out a list and a map in the screenshot and show a screenshot where the hashcodes are fixed or omitted if they are -1.
Hashcodes should also be truncated 5 char hex to match what Flutter does in its debug print.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the split border in the screenshot. I would have expected to see a border that was the focus color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The split border is the thin grey border

But maybe focusColor is not the correct variable to use

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2021-04-01 at 04 35 19

@rrousselGit
Copy link
Contributor Author

Should I skip goldens? Since they generate 0.01% diff, probably due to environment differences.

@rrousselGit
Copy link
Contributor Author

Hum interesting that this time they passed

@kenzieschmoll
Copy link
Member

kenzieschmoll commented Apr 7, 2021

coming in kind of late here but I thought we agreed to not use package:freezed because that isn't a package we want to have rolled into g3 (the preferred solution in g3 is using built_value)? yet I'm still seeing .freezed files throughout this PR? @jacob314 @rrousselGit

@rrousselGit
Copy link
Contributor Author

After discussing with @jacob314 , we agreed to remove the dependency and commit the generated file

Those @freezed are fake annotations, defined as const freezed = Object().
They exist to easily re-generate the files

@kenzieschmoll
Copy link
Member

Will the .freezed files ever need to be regenerated? Perhaps we could include a comment at the top of the .freezed files that describes how to regenerate these files if necessary

@rrousselGit
Copy link
Contributor Author

There is one here:

// real annotation package, then execute `pub run build_runner build`.

Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we sort the list of providers alphabetically? If you are looking for a specific provider, it can be challenging to find what you are looking for at first glance
Screen Shot 2021-04-14 at 1 45 01 PM

color: backgroundColor,
padding: _tilePadding,
child: state.when(
loading: () => const CenteredCircularProgressIndicator(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get rid of these spinners? It looks like there is a single spinner for the whole container, and then there is a spinner for each row. It created a kind of strange multi-spinner effect that we probably don't want:
Screen Shot 2021-04-14 at 12 52 31 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good now

@kenzieschmoll
Copy link
Member

Looking good! a few UI requests and then LGTM

requiresLibrary: 'package:provider/',
title: 'Provider',
requiresDebugBuild: true,
icon: Icons.palette,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to change the icon - not sure if this once was listed in the conditional screens example by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, switched to attach_file

@jacob314
Copy link
Contributor

lgtm
once the few polish comments @kenzieschmoll mentioned are addressed.

Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
As soon as the tests pass lets land this.

@rrousselGit
Copy link
Contributor Author

This should be good to go now

@jacob314 jacob314 merged commit 1e3d42c into flutter:master Apr 29, 2021
@rrousselGit rrousselGit deleted the provider-only branch April 29, 2021 16:48
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

Successfully merging this pull request may close these issues.

4 participants