First pass adding flow annotations.

[ludofischer]

As @chenglou suggested in #180 , here’s a pull request with flow checks added to just a subset of the project files. I am only checking files where Flow recognizes all language constructs. Since Flow already merged the fixes related to const destructuring and export default, it seems pointless to change the project coding style, only to go back when the next Flow version releases.

Among the project dependencies, I ignored some files because we hit facebook/fbjs#44 with React 0.14.0-rc1; I also ignored a dummy package.json file used to test npmconf because flow mistakes it for the real deal.

[chenglou]

Awesome!

[chenglou]

Is it possible theoretically to type check this as something more precise than Object?

[ludofischer]

That’s possible. What additional constraints would you like to enforce?

[chenglou]

Humm on second thought it could be a bit complicated. The thing is, currentStyle looks like this: {height: {val: 10, config: [1, 2]}, width: justANumber, randomKey: someRandomValue}. The randomKey part screws things up. It's needed because for TransitionMotion you might need to carry around a pack of info around so that you can still access them when the item's gone everywhere else. Do you know what I mean?

If this is type-checkable it'd be great. Say it's type A. TransitionMotion's styles would be typed as a map of {stringKey: A}. Avoids some confusion if you use types everywhere.

So right now it's not easily typeable. 2 solutions I can think of:

1. Impose that recognized keys, e.g. height, transform, opacity, etc. Have that Number | ConfigObject type. This isn't cross-platform though.
2. Impose that anything you carry around be in a key called data, of type Any. Everything else should be Number | ConfigObject.

Any other solution? If not, either way we'd need to leave this as Object for now.

[ludofischer]

1. I don’t understand why it isn’t cross-platform. Because you would need to hardcode all the possible keys? But then we would have to track all possible CSS properties. I do not think we should enforce the validity of the CSS property at the type level.
2. I prefer this solution. But couldn’t randomKey become an extra parameter instead of living inside the same configuration object. We would have a config that includes the essentials that every animation needs, and could be precisley type-checked and a separate object for random stuff. Right now the situation is a bit contradictory as the code operates on the style objects with functions that are extremely generic and would almost work for any type of object, yet we also wish to impose constraints on those same objects. 'Isolate the parts that change from those that don’t' might also apply to types.

[chenglou]

1. Well partially, yes. This also has to do with the fact that it's not clear that you'd want height rounded to integer on e.g. react-canvas (if they even care about react-motion).
2. The extra data carried around is only for TransitionMotion. How would you do this:

<TransitionMotion styles={stuff}>{interpolated => Object.keys(interpolated).map(style => randomKeyWithMyData...)}

Passing as an extra param would work well here, would it?

cc @phaistonian

[nkbt]

For now it could be a simple type Options which equals to Object. Later Options can be updated with more specific type. And it will be in one place rather then updating (and possibly missing some) all places in the code that use the same Options

[nkbt]

@chenglou I've first read your commend in the other way and that was funny.

[ludofischer]

I’ve had mixed experiences with type aliases for simple types in Scala. I guess it would not change much whether we define a new Options type in this case. Do you know whether we can export type definitions from a module like any other variable?

[chenglou]

Yes you can. Also, I'm ambivalent as to aliasing it to something else.

[nkbt]

I really think we need alias. It is not about Options being equal to Object, it is about idiomatic function API.

It also gives you ability to do compile-time checks if arguments are actually the same type. So if in one place you will use Options and in the other place Object - check will fail.

Often I have separate PropTypes file that exports something like:

export const id = React.PropTypes.number;
export const name = React.PropTypes.string;

[chenglou]

Humm, I guess it's all set?

[nkbt]

Sorry for nitpicking but I can't recall us using this awkward indentation and it will not pass ESLint check.

Should be something like

export default function stepper(
  frameRate: number,
  x: number,
  v: number,
  destX: number,
  k: number,
  b: number): [number, number] {

Disregard if I'm wrong.

[ludofischer]

It does pass ESLint checks (you might want to change the ESLint config), but I can change it anyway. Is it just the first argument on a new line?

[nkbt]

It is 2 space indentation, and first arg on a new line too.
Thanks.

[chenglou]

So I guess this is good to go? Beside the nits =D

[ludofischer]

I have fixed the indentation issue that @nkbt highlighted and created a Style type. Now that Flow 0.17 is out we can try to check the rest of the code base. Do you want to merge this one right now or do you prefer if I try to annotate more files first?

[ludofischer]

Interestingly, if you update to the latest ESLint and AirBnB style guides, the lint npm task reports 169 errors… that Flow does not care about.

[chenglou]

Do you want to merge this one right now or do you prefer if I try to annotate more files first?
We should merge this right now. I don't understand the lint errors though. Variable names too short? Since when?

[ludofischer]

You get lint errors only if you change the eslint-config-airbnb to 0.1.0, so it’s not something to worry about now. I’ve opened an issue to keep track of this: #211

[chenglou]

Alright, merging! =D

Thanks a lot!

[nkbt]

I was expecting this indentation to be fixed everywhere not only in the place I left my comment ;)

[nkbt]

Sorry for post-review, I somehow missed updates