Add Flow support

[KyleAMathews]

http://flowtype.org/

Type checking finds subtle bugs and makes code (more) self-documenting.

The initial PR for this should be very simple — add necessary dependencies + add types to a few internal APIs. Once we start adding typing to external APIs (e.g. modifyWebpackConfig) then it'd be great to export the types so Gatsby sites could use the types.

If anyone has experience with this, please chime in! Would love to learn more.

Resources:

• Flow tutorial https://github.com/runtastic/flow-guide

[bryannaegele]

I'm interested in getting to know more about Gatsby's core and Flow, so I'd be happy to take this on. Can you compile a list of which internal libs you would want to start with?

[KyleAMathews]

@bryannaegele that'd be fantastic! Always nice to have someone take something off your TODO list :-)

How about for the first PR we focus on getting just one internal API setup correctly and then go from there? So add Flow as a devDependency and add to npm test so it'll be run on TravisCI on PRs, etc.

Probably a good place to start adding types is the code in https://github.com/gatsbyjs/gatsby/blob/master/lib/utils/build-page/ as it's a bit tricky and core to Gatsby so should be bullet-proof.

Future PRs can cover additional parts of Gatsby + create types for the user-facing APIs Gatsby exposes. It'd be 💯 if we could export these types so people could type-check their sites' use of Gatsby's APIs.

Thanks again Bryan! Welcome aboard, we're happy to have ya :-)

[bryannaegele]

That is a sound plan. I'll track progress here. If there's anything else you can think of we can add it.

• Add flow-bin, flow npm script, and call in test
• Setup eslint & Babel to handle flow
• build-page utils - get flow check to pass (add necessary types, annotations, and external interface declarations)
  • index
  • load-frontmatter (awaiting Analysis does not match fs.readFileSync declaration facebook/flow#1870)
  • path-resolver
  • url-resolver

I should have something to review within the week. Hopefully that flow issue I found will get a merge and maintenance release soon.

One potential issue I have found is that flow still does not have up-to-date dedicated support on windows yet. This would prevent a windows contributor to be unable to successfully complete npm test. Not sure if this would be a deal breaker for you. We should at least add a note to contributors about it.

[KyleAMathews]

Sounds great! Thanks again for taking this on!!

Re: Windows, this is not a deal breaker but we should communicate this to devs on Windows wishing to contribute. Perhaps add a npm test-windows command + a note in https://github.com/gatsbyjs/gatsby/blob/master/CONTRIBUTING.md

Or does this mean that the babel flow plugin wouldn't work at all meaning that Windows developers couldn't make changes to the src period? That's not so nice...

[bryannaegele]

They should be able to contribute. They won't benefit from the type checking and probably couldn't effectively add flow checks.

Babel will run fine. It is just stripping flow markup from the files before processing them.

Eslint may harp a bit but I think it will just ignore it since the plugins are more to ignore some flow markup conventions.

I can add a windows specific test that omits the call to the flow script. It would be cool if somebody knew a simple scripting hack to effectively end the script before that command on windows only. I did a quick search but haven't come up with anything too promising.

Nuclide relies on flow being in your path for editor support, so I don't think this could be perfectly solved with Docker. Docker would allow a Windows user to use the CLI and run tests, however.

[MoOx]

Here is what we are doing for phenomic to skip flow check on windows https://github.com/MoOx/phenomic/blob/104fa249b585d88302827af637261099b4fa4689/scripts/flow-check.js

[bryannaegele]

Thanks @MoOx! Seems like a great starting point.

[bryannaegele]

I got to spend some more time on this tonight but Flow is pointing out the trickiness of the build-page lib. Looking back at the history for this module, it seems like it was perhaps a quick breakup of a larger file (glob-pages) by @benstepp, but not a complete refactor.

I'm not confident this module could fully benefit or implement Flow without a refactor, which I think this module could use. Index just composes fragments of the page object from the other modules, but they're all fairly interdependent. Flow doesn't seem to support layering in typing of object maps in a way that the current code would require.

I can take this on if everyone agrees this is worthwhile.

If we proceed, I do have some questions regarding the assignment of undefined to the path property if it is a template and the omission of the templatePath property if it is a template. I don't know the library well yet, but it seems the path property could be reused rather than omitting templatePath or assigning undefined to path. It seems the returned object could also simply declare what type of object it represents (page or template) rather than checking filenames again later.

[KyleAMathews]

Everything you're suggesting here seems reasonable. I don't feel married to the current implementation at all so if you see need for refactorings to clean things up or to make Flow happy, feel free. @benstepp any thoughts? @gatsbyjs/gatsby-core-maintainers

[scottnonnenberg]

I'm ambivalent about Flow and really any attempt to make Javascript more type-safe. I think tests are way more important. I'd do that first. And I may still! :0) I'm preparing my blog for open-source release now, and it has pretty good tests - basic unit tests and React component tests with Enzyme...

(funny enough, just today encountered Dan Luu's post about type-safety: http://danluu.com/empirical-pl/)

[KyleAMathews]

@benstepp has been adding a bunch of tests recently! Still more to do there so feel free to jump in Scott :-)

Re: Flow & types. I dislike heavy-handed type systems but from what I've heard, Flow is pretty minimal and flexible so it seems like a safe thing to add. If it proves otherwise, we can just rip it out :-) I don't think there's any silver bullet for software correctness/robustness so using multiple tactics towards that end seems wise.

[KyleAMathews]

@bryannaegele how are things? For a first PR, how about just adding the most basic Flow support e.g. flow-bin, the eslint & babel plugins, add a .flowconfig, and add /* @flow */ to each js file. After that's in, we add PRs that gradually add static typing (and fix errors like you found above as we go). I ask as I have some new Gatsby stuff in mind that I'd love to start playing with Flow plus have it around to guide my prototyping. Thanks!

[bryannaegele]

Apologies @KyleAMathews. I've been slammed with a project at work. Luckily, it involves many hard-learned lessons with flow.

I can push a PR with the very basic setup later this week. We will need to refrain from running flow check as part of the test script if you want to add the flow flag to every file, as adding flow with even just the weak setting will generate a ton of errors.

So the two options I can surmise are:

1. Add /* @flow weak */ to every file and not run flow check with the test script.
2. Add flow check to the test script but refrain from adding flow to every file.

The advantage to option 1 is you could have a starting point to create issues from. An advantage to option 2 is you aren't relying on contributors to manually check for errors.

The narrowed scope does seem like a safer and less intrusive approach to get started. I also like the idea of splitting any refactor to the build-page module to a separate issue.

[KyleAMathews]

Haha :-) sorry about that but lucky us :-)

Let's start with 2. I like having having things enforced by TravisCI as soon as possible. Also that'll give other people following along opportunities to jump in and contribute small Flow PRs if they'd like.

Re: narrowed scope. This is something I've been telling myself a lot recently as well as to break things into smaller chunks. I have to fit in Gatsby work between regular work so the smaller the chunks, the more regular the progress.