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

Is there a way to correctly type check this? #673

Closed
leoasis opened this issue Jul 30, 2015 · 10 comments
Closed

Is there a way to correctly type check this? #673

leoasis opened this issue Jul 30, 2015 · 10 comments
Labels

Comments

@leoasis
Copy link

leoasis commented Jul 30, 2015

I'm trying to figure out how to correctly type functions that accept an object literal as a record of functions and return another record with the same properties but values that have a type that depends on the type from the parameter's property.

Some concrete examples:

// Takes an object as a record with values that are functions with different arguments each (possibly), and a function to bind, and returns an object with the same properties, with those functions that were values bound to call that second parameter function
function bindActionCreators(actionCreators, dispatch) {
  return mapValues(actionCreators, actionCreator => {
    return (...args) => dispatch(actionCreator(...args));
  });
}
// Accepts an object with properties whose values are functions of type: (State, Action) => State (in this example, Action type is the same among reducers, but State can be of different type
// and returns a reducer function of type (CombinedState, Action) => CombinedState, where CombinedState is an object whose properties are the same as the object passed to `combinedReducers`, and the values are the accumulated values of each reducer function.
combineReducers(reducerFn) => reducerFn

// Example
function foo(state: number, action: Action): number {...}
function bar(state: string, action: Action): string {...}
var reducer = combineReducers({foo, bar})

var initalState = {foo: 0, bar: ''};
reducer(initialState, {some: 'action'}); //=> {foo: 3, bar: 'whatever'}

It seems kind of related to this issue, I think: #362.
Not sure how this is actually solved in other type systems, or if it is even possible to do.

Any help or guidance regarding how to type this, or to know if it is even possible to type this, would be awesome!

@popham
Copy link
Contributor

popham commented Jul 30, 2015

Your first example is confusing me, so I'm ignoring it (mapValues~combineReducers?).

With respect to the second, I expect that disjoint union types make short work of this, as long as you're okay with Flow inferring the function types. Just provide a combineReducers implementation, i.e. swap function combineReducers(reducerFn) {...} into the combineReducers(reducerFn) => ReducerFn spot, and add

type Action = "action" | "anotherAction" | ...

This smells a lot like a pattern heavily used in Flux, so I would search for Flow-typed Flux stuff for analogous code.

@leoasis
Copy link
Author

leoasis commented Jul 30, 2015

@popham in the first example, mapValues is actually Lodash's mapValues function. With that is it clear enough?

As for the second one, inferring is not working, and I think it's due to, AFAIK, the fact that it's in a separate module and Flow needs you to have types in the modules boundaries.

It actually is sort-of Flux code, I want to solve this problem in this effort to provide Flow types for the redux library: reduxjs/redux#356

@popham
Copy link
Contributor

popham commented Jul 30, 2015

@leoasis lets focus on the second one, and the solution should extrapolate.

Have you tested the distinction? That is, have you tried this with everything in a single module?

@leoasis
Copy link
Author

leoasis commented Jul 30, 2015

I've just tried and it seems to be working correctly.

This is the real implementation of that function I'm talking about: https://github.com/leoasis/redux/blob/bec3cb222f844f148bd0ed671629ae218ba7459f/src/utils/combineReducers.js

As you see, it has a lot of dependencies as well which makes the type inference no longer work. I've replaced that function in my example with this one, which does more or less the same:

function combineReducers(reducers) {
  return function combination(state = {}, action) {
    return Object.keys(reducers).reduce((fullState, key) => {
      var reducer = reducers[key];
      var newState = reducer(state[key], action);
      fullState[key] = newState;
      return fullState;
    }, {});
  };
}

And it correctly type checks, but only in the same file. Is there a way to achieve that behavior across modules? Or, is there a type signature for this function that provides the same behavior?

@popham
Copy link
Contributor

popham commented Jul 30, 2015

By "working correctly", I'm assuming that you mean Flow processes your module with 0 errors. I'm also assuming that in the imports case you're importing a modules that contains the above function as a default, export default function combineReducers..., and that you've removed all of the imports.

I think that it's safe to declare this a bug in Flow's import/export mechanics.

  • To work around your issue, I would
    • try a named export instead and then
    • make liberal use of any and TODOs (I think $FlowFixMe is relevant here also, but I've never used it).
  • To get this fixed as soon as possible, I would change the name of this thread to something likely to draw the attention of a maintainer who is interested in all-things-module (I think that's @jeffmo). Also, your testing work above would be useful to somebody fixing this--I would attach it as a gist-link on this thread.

@popham
Copy link
Contributor

popham commented Jul 30, 2015

(I just noticed that your file doesn't have a @flow directive at the top. You are type checking the imported module, right?)

@leoasis
Copy link
Author

leoasis commented Jul 31, 2015

@popham First of all, thanks a lot for your support on this issue (haven't thanked you before :)). Now continuing...

By "working correctly" I mean that it throws 0 errors when the types are correct, and throws type errors when they're not.

Will try what you mentioned about not using the default export, and get back here.

Regarding your second comment about the @flow directive, I've tried that but it's not in that commit that I liked to, it's still part of some local changes I have. What happens when I put the @flow directive in the file is that flow throws the Missing annotation error for that exported function.

Anyway, I'd really like to know if there's a type signature that will make that code behave as if it was inferred, so at least the type is documented.

Thanks again for your help!

@popham
Copy link
Contributor

popham commented Jul 31, 2015

Doh. I forgot about the module boundary limitation. All exported stuff needs to be annotated.

I think you're stuck with anys here:

type Action = "Action1" | "Action2";
type<U> Er = (state: U, action: Action) => U;
type ActionSet = {
  [key: string]: Er // This effectively takes Er<any> for each member
};

function combineReducers(reducers) {
  return function combination(state = {}, action: ActionSet): any { // Stuck with `any` here
    return Object.keys(reducers).reduce((fullState, key) => {
      var reducer = reducers[key];
      var newState = reducer(state[key], action);
      fullState[key] = newState;
      return fullState;
    }, {});
  };
}

I'm having trouble even imagining a tidy syntax that could support this. I would suggest rearchitecting the code to contain the state machine within a class, but I'm betting that runs contrary to your goals with respect to that library.

@leoasis
Copy link
Author

leoasis commented Aug 3, 2015

I'm having trouble even imagining a tidy syntax that could support this.

@popham yeah I was trying to see if this is even possible in type systems such as in Haskell, and if not, what's the alternative for an equivalent function or something with similar behavior.

Will try your approach with any and see how far I can get without making the output just any, and explicitly "cast" the output to the correct type. For now I think that shouldn't be that bad, but it would be great if there was a solution, even if not yet implemented in Flow, to solve this.

@nmn
Copy link
Contributor

nmn commented Mar 4, 2017

This has been partially resolved with utility types such as $ObjMap. Researching more about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants