rrousselGit / state_notifier

ValueNotifier, but outside Flutter and with some extra perks
MIT License
311 stars 28 forks source link

Remove `get state`'s protected status #69

Closed willlockwood closed 2 years ago

willlockwood commented 2 years ago

Is your feature request related to a problem? Please describe.

Often, I use StateNotifiers in various services in a Flutter app, that need to get some information about the current state of the notifier. With ValueNotifier, this is easy—you can just do notifier.value wherever you'd like, and you have the access you need. With StateNotifier, it's technically possible via notifier.state, but only if you ignore the fact that the getter for state is protected in StateNotifier.

My understanding is that the decision to make state protected in StateNotifier was to prevent mutating the state outside of the notifier. This is at least what the docs seem to suggest. However, in this case, I don't need to mutate anything—it's only the getter that I want to be able to access outside of the class, and it should be possible to allow for reading state without having to ignore warnings intended to prevent something I'm not doing.

Describe the solution you'd like

Currently, users will get warnings for accessing both the getter and setter for state outside of the notifier:

image

The most straightforward solution is to remove the @protected annotation from the getter:

image

Describe alternatives you've considered

There are a few ways to get around this now, without changing StateNotifier—I just think they're all slightly unfortunate:

1. You can just ignore the warning

This can be fine, but if there are multiple places in a file that you want to access the getter, you either have to add a ton of ignores for each line, or add an ignore for the whole file, which can be dangerous, as it would also ignore the warning for other objects in the file. Also, I generally would think that "just ignore the warning" is a not really a great solution for anything, if it can be avoided.

2. Override the getter on the notifier or add your own

This can also be fine, but when you have to do this for 20-30 StateNotifiers in an app, it becomes tiresome, and feels wrong. I wouldn't usually encourage any of my teammates to be overriding already-implemented methods in our core architecture dependencies:

image image

3. Use addListener, which seems to be the main intended way to access state from a StateNotifier

This, in some cases, doesn't even work. In the example above, you'll need to keep track of state locally in the service, and update it when the state changes. In the example below, you see that it becomes difficult to be sure that the local copy of the state has actually been initialized, and also, it becomes a bit more involved when you need to be sure to remove the listener as well. It seems like addListener is the main intended way to access state from a StateNotifier, but in this case, it's the most difficult solution to get working.

image

Additional context This isn't urgent, but I think it would be an improvement. It definitely comes up in my workflow a lot, and I usually struggle to come to a good decision about what workaround I like most for the context at hand. Sometimes, I just use ValueNotifier so I don't have to worry about it.

I'm happy to make this change myself, if you agree that this is worth updating?

willlockwood commented 2 years ago

Ah whoops—just noticed right now that I actually got a response on this idea already, outside of github—my mistake.