Hydrogen: Add flow types to remaining files

[lgeiger]

We've added flow types to almost every file. Thanks @BenRussert for the great work here! 👏

There are only two files remaining: 😮 🎉

• [ ] [jupyter-js-services-shim.js](https://github.com/nteract/hydrogen/blob/master/lib/jupyter-js-services-shim.js)
• [ ] [kernel-manager.js](https://github.com/nteract/hydrogen/blob/master/lib/kernel-manager.js)

[jdpigeon]

I think I can find time before the sprint to add flow to these files. I've been meaning to learn it and this looks like a perfect opportunity!

[lgeiger]

@jdpigeon Very cool!

[BenRussert]

@jdpigeon please don't hesitate to ask questions here or on slack!

[jdpigeon]

What's best practices when describin the flow type of a parameter that is actually a function?

In the first method in kernel-manager.js, I used some detective work to find out that the first parameter, grammar, should have the type grammer: atom$Grammar. However the second argument seems to be a function! Can I just set it to onStarted: Function?

startKernelFor(grammar: atom$Grammar, onStarted: Function) {
    try {
      const rootDirectory = atom.project.rootDirectories[0]
...

[BenRussert]

@jdpigeon What you have is fine, looks like you are on the right track! Depending on the situation you can elaborate w/ function parameters, for example if you needed the given function to take certain types as arguments, or return specific types.

For more on this check out:, function types

[lgeiger]

@BenRussert Thanks for the link. We're using Function quite often in our typings, we should change that.

@jdpigeon I think something like this should be fine:

startKernelFor(grammar: atom$Grammar, onStarted: (kernel: Kernel) => void) {

By the way if you're searching for a clean flow linter for Atom, I can recommend flow-ide.

[jdpigeon]

Nice! That makes this quite easier

[jdpigeon]

Hey guys,

So I went through and added Flow types to those two files. However, in trying to resolve linting issues that came up, I got to the point where it seems like changes to the actual code need to be made. Since I'm not familiar with the code base, I'm gonna pass my changes off to you guys for support.

Specifically, adding Flow to kernel-manager.js created a number of errors both within the file and others (main, signal-list-view, ws-kernel, and zmq-kernel), mainly related to:

• expected parameter types of things imported from the store (store.grammar throws a 'null type is incompatible with atom$Grammar' bug)
• Functions with two few arguments (ie. ZMQKernel constructor). Not sure if parameters should be nullable and, if so, how to account for that.

All the changes I've made are in this commit on my personal fork: https://github.com/jdpigeon/hydrogen/commit/ba4d41b60327a5f8c565ddaa6c46765573c7ef46

I left comments near many of the errors w/ points of confusion

[BenRussert]

@jdpigeon very cool! I think you should submit a PR so we can cover your questions in review and get this merged. You can even put your own comments in the pr next to the lines you have questions about.

I'll review next chance I get and @lgeiger usually has helpful input as well.

[BenRussert]

@jdpigeon want to submit as a pull request?

[jdpigeon]

Got it https://github.com/nteract/hydrogen/pull/832

[BenRussert]

Closed since #832, thanks @jdpigeon!