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

TypedMiddleware not accept specified middleware type. #35

Closed
jaggerwang opened this issue Jun 18, 2018 · 8 comments
Closed

TypedMiddleware not accept specified middleware type. #35

jaggerwang opened this issue Jun 18, 2018 · 8 comments

Comments

@jaggerwang
Copy link

Problem

When specify the exact action type, TypedMiddleware complains [dart] The return type '(Store<AppState>, LoginAction, (dynamic) → void) → Null' isn't a '(Store<AppState>, dynamic, (dynamic) → void) → void', as defined by the method 'loginMiddleware'..

Code

List<Middleware<AppState>> createMiddlewares({
  AccountService accountService,
}) {
  accountService = accountService ?? AccountService();

  return [
    TypedMiddleware<AppState, LoginAction>(loginMiddleware(accountService)),
  ];
}

Middleware<AppState> loginMiddleware(AccountService accountService) =>
    // Specify the exact type of action, not dynamic
    (Store<AppState> store, LoginAction action, NextDispatcher next) {
      next(action);
    };

Env

name: tangbole
description: A tumblr client app.

dependencies:
  cupertino_icons: ^0.1.2
  english_words: ^3.1.0
  flutter:
    sdk: flutter
  flutter_redux: ^0.5.1
  http: ^0.11.3+16
  meta: ^1.1.5
  uri: ^0.11.2
  redux: ^3.0.0
  redux_logging: ^0.3.0
  redux_persist: ^0.7.0-rc.2
  redux_persist_flutter: ^0.6.0-rc.1

dev_dependencies:
  flutter_driver:
    sdk: flutter
  flutter_test:
    sdk: flutter
  mockito: ^2.2.3

flutter:
  uses-material-design: true
  assets:
    - images/lake.jpg
@brianegan
Copy link
Collaborator

brianegan commented Jun 18, 2018

What if you try this? This is similar to your other problem: Dart is inferring the more specific type information that TypedMiddleware provides rather than the more abstract Middleware<AppState>.

List<Middleware<AppState>> createMiddlewares({
  AccountService accountService,
}) {
  accountService = accountService ?? AccountService();

  return <Middleware<AppState>>[
    TypedMiddleware<AppState, LoginAction>(loginMiddleware(accountService)),
  ];
}

@jaggerwang
Copy link
Author

jaggerwang commented Jun 18, 2018

Because I want use code assistant for action! Using type dynamic lost the exact action type information, and there is no code assistant.

The action type param is already given to TypedMiddleware, so why not using the exact middleware type?

@brianegan
Copy link
Collaborator

brianegan commented Jun 18, 2018

I'd ask: Please let us be kind and patient with one another. I'm doing my best to help you with the very limited amount of free time I have. In this case, your app has a small type error. Could you please try this and see if it works?

I realized the mistake is in a slightly different part of your code. Please change this:

Middleware<AppState> loginMiddleware(AccountService accountService) =>
    // Specify the exact type of action, not dynamic
    (Store<AppState> store, LoginAction action, NextDispatcher next) {
      next(action);
    };

to this:

void Function(
    Store<State> store,
    LoginAction action,
    NextDispatcher next,
  ) loginMiddleware(AccountService accountService) =>
    (Store<AppState> store, LoginAction action, NextDispatcher next) {
      next(action);
    };

In this case, you want to return a Function, but you don't want to return a Middleware<AppState> function because it does not match what TypedMiddleware is expecting (TypedMiddleware does not accept a Middleware<AppState> function).

In this case, you need to conform to what the TypedMiddleware constructor expects: A function with more specific type info.

@jaggerwang
Copy link
Author

Sorry, I read this "What if you try this?" to "Why if you try this?":)

Anyway, using the Function way works, but it's not a elegant way. Right now I'm using the following way.

Middleware<AppState> loginMiddleware(AccountService accountService) =>
    (Store<AppState> store, dynamic action, NextDispatcher next) {
      final a = action as LoginAction;
      accountService.login(a.username, a.password).then((response) {
        if (response.code == TangboleApiResponseCode.ok) {
          store.dispatch(
              LoginSucceededAction(UserModel.fromJson(response.data['user'])));
        } else {
          store.dispatch(NotifyAction(NoticeModel(message: response.message)));
        }
      });
    };

@brianegan
Copy link
Collaborator

brianegan commented Jun 18, 2018

No problem at all.

As a heads up: your current solution might lead to runtime errors. If the action is not a LoginAction, but a LogoutAction, you'll get a CastError when the Middleware attempts to convert a LogoutAction to a LoginAction on this line: final a = action as LoginAction;. Therefore, I'd recommend using an if statement: if (action is LoginAction) { /* rest of code here */ } or another option: Extend TypedMiddleware instead. It would look something like this (might need to be adjust slightly as I don't have your full class tree):

class MyMiddleware extends TypedMiddleware<AppState, LoginAction> {
  MyMiddleware(AccountService service)
      : super(
          (
            Store<AppState> store,
            LoginAction action,
            NextDispatcher next,
          ) {
            service.login(a.username, a.password).then((response) {
              if (response.code == 200) {
                store.dispatch(LoginSucceededAction(
                    UserModel.fromJson(response.data['user'])));
              } else {
                store.dispatch(
                    NotifyAction(NoticeModel(message: response.message)));
              }
            });
          },
        );
}

@brianegan
Copy link
Collaborator

Since we solved the original problem, I'll go ahead and close this issue. Please let me know if ya need more help :)

@jaggerwang
Copy link
Author

jaggerwang commented Jun 19, 2018

There is another way.

List<Middleware<AppState>> createMiddlewares({
  AccountService accountService,
}) {
  accountService = accountService ?? AccountService();

  return [
    TypedMiddleware<AppState, LoginAction>(loginMiddleware(accountService)),
  ];
}

dynamic loginMiddleware(AccountService accountService) =>
    (Store<AppState> store, LoginAction action, NextDispatcher next) {
      accountService.login(action.username, action.password).then((response) {
        if (response.code == TangboleApiResponseCode.ok) {
          store.dispatch(
              LoginSucceededAction(UserModel.fromJson(response.data['user'])));
        } else {
          store.dispatch(NotifyAction(NoticeModel(message: response.message)));
        }
      });
    };

As TypedMiddleware make sure to pass the right type of action to loginMiddleware(), so we can simply using return type dynamic to allow specify the exact action type.

Dart not support generic param extending? As type (Store<AppState> store, LoginAction action, NextDispatcher next) is not a subtype of (Store<AppState> store, dynamic action, NextDispatcher next).

@brianegan
Copy link
Collaborator

brianegan commented Jun 19, 2018

"As TypedMiddleware make sure to pass the right type of action to loginMiddleware(), so we can simply using return type dynamic to allow specify the exact action type."

Sure, that works. I'd personally use the full type info rather than dynamic, but if ya like that better then go for it.

"Dart not support generic param extending?"

We might need to look at changing this -- In Dart 1, dynamic was a "base type" for all objects. In Dart 2, it isn't. Therefore, it might make a bit more sense to use Object instead. However, I'd have to test if that would resolve the type issue or if Dart would still complain.

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

2 participants