fluttercommunity / redux.dart

Redux for Dart
https://pub.dev/packages/redux
MIT License
516 stars 61 forks source link

Use optional type instead of dynamic for actions #77

Open hacker1024 opened 3 years ago

hacker1024 commented 3 years ago

Proposal

It'd be great if Store had another generic type to be used as the action argument type in methods like dispatch, preventing non-action objects from being used as actions.

This should be non-breaking for most users, as type inference should allow Store constructors to omit the existing state type, and the addition of another type will only break invocations that specify it explicitly.

Reasoning

I've separated my project into layers, like so:

Data: project_data: Platform-agnostic repository implementations

Domain: project_domain: Core platform-agnostic functionality, including state management

Platform: project_flutter: A GUI using Flutter project_cli: A CLI

Platform implementations can build a Redux store with a buildStore function in the domain package, passing in repository implementations to use. The domain package exports different action classes that the platform implementation can dispatch to the store. Each action class extends from an internal (non-exported) AppAction class.

If the dispatch function only accepts AppAction subtypes, this will prevent non-action objects being used by the platform implementations, requiring all actions to be used from the domain package.

Workarounds

At the moment, an assert can be used, but this throws an error at runtime. A compilation error is preferred.

AppState appReducer(AppState state, dynamic action) {
  assert(action is AppAction,
      'The action object ($action) has an invalid type (${action.runtimeType}! It must extend AppAction.');

  // ...
}
brianegan commented 3 years ago

This should be non-breaking for most users, as type inference should allow Store constructors to omit the existing state type, and the addition of another type will only break invocations that specify it explicitly.

The reason we use dynamic instead of a typed generic is to support 3rd party middleware that need to dispatch custom actions. For example, the redux_dev_tools library dispatches custom actions to support time travel.

Therefore, this would be breaking change and a significant one at that. It's something we considered early on, but we chose not to go with a typed action because many middleware packages need to dispatch custom actions, which would not work in a typed-generic world.

hacker1024 commented 3 years ago

dynamic could still be used by those who use third-party packages, but as a large amount of people use third party packages, a change like this does seem like a lot of effort for very little gain. I understand why it may not be worth it. Feel free to close this issue.

brianegan commented 3 years ago

For sure -- there are pros and cons to allowing and not allowing generics :)

It's been a while since we considered the topic. Happy to leave it open for a bit to hear from more folks if they're interested!

MichaelMarner commented 3 years ago

Just a datapoint - we do not use a parent class for any of our actions when using Redux in Flutter. We do in Angular, but that's because we need all our actions to have a type property. This isn't needed in Dart.

I can't think of a use case where all actions would have some shared functionality where we would require extending from a base class. Unless you're using some uber AppAction that handles everything, but I'd consider that an anti pattern and what you should be doing is having many, single purpose actions.

It's not just middleware this would cause problems with, but also libraries like in redux_entity. I'm not sure it would be possible to use redux_entity in a project that enforced a base class for actions.

I'm also not sure that an assert to ensure the type of the base class is a good idea. What you want your reducer to do, in the case of an unknown action, is to NOP and just return the state unchanged. That way your reducer won't break if the app changes and new kinds of actions flow through the store.

tl;dr - I can't think of a practical benefit this would bring for how we are using redux, and would cause problems for packages.

spkersten commented 3 years ago

Could it be Object instead of dynamic to exclude null and improve the static type safety?

hedgepigdaniel commented 1 year ago

Having an Action parameter would be fantastic.

Yes, middlewares complicate things:

But these problems are surely worth solving - the payoff is that dispatching an action can be type checked, so you can actually reason about what actions flow through the store. At the moment there's nothing stopping you from dispatching an action that isn't handled by the reducers (including by ignoring it...), and no help in the reducer type to understand what the action argument actually might be.

Seems like part of the problem is the lack of union types in Dart (see https://github.com/dart-lang/language/issues/83) - which makes it hard to say that an action could be SomeAction | AnotherAction - instead they all have to inherit from a single class.

Maksimka101 commented 1 year ago

I agree that dynamic is bad thing. So I created a new typed redux packages for the redux, flutter_redux and redux_epics. typed_redux, flutter_typed_redux and typed_redux_epics

Maybe it will help to anybody else : )