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

Add Flowtype (WIP) #530

Merged
merged 4 commits into from
Aug 23, 2016
Merged

Add Flowtype (WIP) #530

merged 4 commits into from
Aug 23, 2016

Conversation

AlexKVal
Copy link
Contributor

@AlexKVal AlexKVal commented Aug 21, 2016

I will be gradually adding commits step by step to facilitate review and for earlier feedbacks.

I've started to learn flow. Yay _o_/ 😄

But IMHO
Adding flowtype annotations to the library is kinda Pandora's box.
It adds a lot of additional maintenance burden.

The first three commits:
green https://travis-ci.org/AlexKVal/react_on_rails/builds/153954865

src/Authenticity.js
without {content?: string} it warns about

property `content` Property not found in HTMLElement

because it is a dynamic property from document.querySelector('meta[name="csrf-token"]')

The line
lib/generators/react_on_rails/templates/node/base/client/node/server.js has been added to .eslintignore because of #469 merging with linting errors 😈
I'll remove it after the master branch will have resolved linting issues.

Notes about 'fsevents' bug

After a fresh npm i installation of all packages with node@6 version
running of npm run build or build-watch
throws this node's error:

v8::ObjectTemplate::Set() with non-primitive values is deprecated

babel-cli uses chokidar which uses not the latest fsevents
npm i fsevents@latest command solves the problem.
The cause of it fsevents/fsevents#128
Probably we should add the latest fsevents version to package.json
via npm i -D fsevents@latest command.

Notes about Atom's linter-flow

I've managed to get it working only with node@6.3.1 version (out of 6th versions)
linter-flow doesn't work with the latest (at the moment) node@6.4.0. (w/o errors. it just doesn't show any warnings in Atom)


This change is Reviewable

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9af0cd8 on AlexKVal:flow into * on shakacode:master*.

@alexeuler
Copy link
Contributor

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


node_package/src/context.js, line 5 [r1] (raw file):

 * @returns {boolean|Window|*|context}
 * @flow
 */

I think it might be better to put // @flow as a separate line at the top, o/w it's lost in the comment description.


Comments from Reviewable

@justin808
Copy link
Member

@AlexKVal Did we not need to add the params for flow?

We should probably have this in the eslintrc, but only for the library, not the samples...

  flowtype/require-parameter-type: 2
  flowtype/require-return-type:
    - 2
    - always
    -
      excludeArrowFunctions: expressionsOnly
  flowtype/space-after-type-colon:
    - 2
    - always
  flowtype/space-before-type-colon:
    - 2
    - never
  flowtype/require-valid-file-annotation:
    - 2
    - always
  flowtype/valid-syntax: 2

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


node_package/src/context.js, line 5 [r1] (raw file):

Previously, alleycat-at-git (Alexey) wrote…

I think it might be better to put // @flow as a separate line at the top, o/w it's lost in the comment description.

I agree with

Comments from Reviewable

@justin808
Copy link
Member

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


node_package/src/Authenticity.js, line 10 [r1] (raw file):

  },

  authenticityHeaders(otherHeaders: Object = {}) {

This needs to be string values in the object.


Comments from Reviewable

@AlexKVal
Copy link
Contributor Author

@alleycat-at-git thank you for the help. I wasn't sure what is the best way.

@justin808 thank you for the rules for 'eslint'. I'll implement them one by one.

src/Authenticity.js
without {content?: string} it warns about
> property `content` Property not found in HTMLElement
we don't need additional `console.history.length === 0` check
```js
let line = []
line.join('\n') // results in ''
```
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.609% when pulling 7cac593 on AlexKVal:flow into 5bae865 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.609% when pulling a07244e on AlexKVal:flow into 5bae865 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.609% when pulling a07244e on AlexKVal:flow into 5bae865 on shakacode:master.

@justin808
Copy link
Member

Nice work. A couple ideas.


Reviewed 4 of 5 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


node_package/src/Authenticity.js, line 10 [r3] (raw file):

  },

  authenticityHeaders(otherHeaders: {[id:string]: string} = {}) {

It seems a bit odd for object properties to be a string.

However, this is from the example (kval), so maybe we use key

type MapOfNumbers = { [key: string]: number };
var numbers: MapOfNumbers = {
  ten: 10,
  twenty: 20,
};

node_package/src/generatorFunction.js, line 12 [r3] (raw file):

 * @returns {boolean}
 */
export default function generatorFunction(component: any) {

https://flowtype.org/docs/union-intersection-types.html#_

This one is either

React.createClass (probably an object)
ES6 Class
Function

Then again, any might be good enough.


Comments from Reviewable

@AlexKVal
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions.


node_package/src/Authenticity.js, line 10 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

It seems a bit odd for object properties to be a string.

However, this is from the example (kval), so maybe we use key

type MapOfNumbers = { [key: string]: number };
var numbers: MapOfNumbers = {
  ten: 10,
  twenty: 20,
};
Yes I just got it from `flowtype.org/examples` as is.

node_package/src/generatorFunction.js, line 12 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

https://flowtype.org/docs/union-intersection-types.html#_

This one is either

React.createClass (probably an object)
ES6 Class
Function

Then again, any might be good enough.

I have no idea (yet) how to describe such complex types in `flowtype`. My goal is to satisfy all warnings the simplest possible way and only then start to re-write it with more elaborate and complex flowtype's descriptors.

I'm implementing flowtype first time ever. I have no experience with it at all.
So I really appreciate all the help! Thank you. 🍒


Comments from Reviewable

@justin808 justin808 merged commit f0058d4 into shakacode:master Aug 23, 2016
@AlexKVal AlexKVal deleted the flow branch August 23, 2016 10:16
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

Successfully merging this pull request may close these issues.

None yet

4 participants