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

Use optional type instead of dynamic for actions #77

Open
hacker1024 opened this issue Mar 23, 2021 · 7 comments
Open

Use optional type instead of dynamic for actions #77

hacker1024 opened this issue Mar 23, 2021 · 7 comments

Comments

@hacker1024
Copy link

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
Copy link
Collaborator

brianegan commented Mar 23, 2021

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
Copy link
Author

hacker1024 commented Mar 23, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

MichaelMarner commented Mar 24, 2021

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
Copy link

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

@hedgepigdaniel
Copy link

Having an Action parameter would be fantastic.

Yes, middlewares complicate things:

  • they impose restrictions on the type of action which the user might need to be aware of when defining the action type
  • they potentially create a difference between input actions (actions that can be dispatched), and output actions (actions that can be consumed by reducers).

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 dart-lang/language#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
Copy link

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 : )

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

6 participants