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

ChangeNotifierProvider vs Provider lint? #102

Closed
filiph opened this issue May 30, 2019 · 10 comments
Closed

ChangeNotifierProvider vs Provider lint? #102

filiph opened this issue May 30, 2019 · 10 comments

Comments

@filiph
Copy link
Collaborator

filiph commented May 30, 2019

Someone just showed me provider code that didn't work for them — things were getting updated internally, the ChangeNotifier was calling notifyListeners, the widget was correctly wrapped in a relevant Consumer, there were no static or dynamic errors, but nothing changed in the UI.

The culprit, which took me several minutes to realize, was using Provider instead of ChangeNotifierProvider.

I don't think there is an easy solution for this, but I thought maybe others would have ideas.

@filiph
Copy link
Collaborator Author

filiph commented May 30, 2019

One idea is to have a linter hint which would fire when you have an instance of ChangeNotifier as the value provided by the vanilla Provider.

@rrousselGit
Copy link
Owner

rrousselGit commented May 30, 2019

I'm mixed about the linter. It is a good idea but may not be used.

The main issue with custom analyzer plug-ins is that they require a separate installation step beside the pubspec dependency.

From my experience, most people don't know/have an analysis_options.yaml, and hardly any existing package offers an analyzer plug-in.
So, while possible, I doubt if its effectiveness.

@rrousselGit
Copy link
Owner

rrousselGit commented May 30, 2019

One alternative is to add an assert on Provider similar to assert(value is! Listenable && value is! Stream && value is! StreamController).

There's technically a use-case for it as we're speaking, but:

rrousselGit added a commit that referenced this issue Jun 1, 2019
This is an health check to ensure that the listenable objects
are indeed listened.

closes #102
@listepo
Copy link

listepo commented Mar 22, 2020

I think linter is a good idea

@znjameswu
Copy link

znjameswu commented Oct 23, 2020

I think linter is a better idea. I always use Provider to perform all kinds of dependency injection, including injecting AnimationController and various other stuff that may implement Listenable. To avoid an error, I have to always create a wrapper object.

Setting a global Provider.debugCheckInvalidValueType is just unacceptable for any library code. I think there needs to be a library-friendly solution. Adding an optional parameter in Provider constructor to explicitly allow Listenable seems to be a choice as well.

@rrousselGit
Copy link
Owner

A linter is very time-consuming to implement and the custom lint API makes it that most users would not install the linter, rendering it not very useful.

@listepo
Copy link

listepo commented Oct 26, 2020

I think a linter plugin(like Working with Plugins) would be the best option, but I'm not sure there is such a possibility now

@rrousselGit
Copy link
Owner

There's nothing close to true plug-ins in dart.

There's something WIP, but that consumes a lot of resources (plug-ins have to re-analyze the project). They don't work with "dart analyze" or // ignore. And they need to be installed separately

@listepo
Copy link

listepo commented Oct 26, 2020

I understand you, I am now thinking about implementation using code generation, but I agree that there are many problems

@znjameswu
Copy link

What about an optional parameter inside the Provider constructor, say bool disableInvalidValueTypeCheck = false?

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

No branches or pull requests

4 participants