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

Investigate gradual typing options for JS #522

Closed
alicewriteswrongs opened this issue Jun 10, 2016 · 19 comments · Fixed by #545
Closed

Investigate gradual typing options for JS #522

alicewriteswrongs opened this issue Jun 10, 2016 · 19 comments · Fixed by #545

Comments

@alicewriteswrongs
Copy link
Contributor

At PyCon there was a lot of excitement about gradual typing in Python, and the libraries that let you start checking type annotations in Python code, such as mypy and pytype.

I think it's worth investigating options for gradual typing in JavaScript, we have several functions (especially in util/util.js and util/validation.js) that are used now in several components, as well as several components that are rendered in different contexts. Adding type annotations to the 'core functionality' of our app could give us a lot more confidence about the correctness of our code.

I've done a little bit of initial investigation into using the Flow typechecker for JS. I think it's well-suited to our needs, in that it's possible to gradually add typing annotations to an existing project and it's not necessary to type check the whole project. My understanding is that something like TypeScript would require a more thorough overhaul and tooling change, although that may be a misconception due to focusing on Flow.

I've added Flow to a personal project on this branch: alicewriteswrongs/site#12
There's a lot of other random noise in that PR (I'm messy on personal projects, sue me) but if you look at the changes to Nav.js and a couple of other files you can see an example of what the type annotations for Flow look like, as well as the tooling changes that were necessary.

I found the tooling for using it to be fairly straightforward to use. There's a babel plugin that will automatically strip out the type annotations when transpiling code, and there's also an ESLint plugin to set rules for linting the type annotations. The typechecker itself is installed via npm and run from the command-line, so I think it would be straightforward to integrate into our test suite.

I'm unclear on how much work it would be to actually get to a useful level of annotation on our project. With Flow you opt-in to typechecking on certain files by adding a // @flow comment at the top of the file, so we could probably make some minimal changes to get the whole project passing the typechecker and then add types and add files to the checker as we proceed and as we see fit.

I'd love to know what other folks writing lots of JS think! Do we think this would be helpful? Would it get in the way? Is there another, better option for annotating JS types that would work well with our current tooling?

@alicewriteswrongs
Copy link
Contributor Author

@noisecapella I opened a branch, installed the flow tooling and started playing with it a little bit. It's kind of fun! in a way, haha.

Anyway if you're interested in checking it out I pushed it to GitHub, but haven't opened a PR. The branch name is ap/flow-js if you want to check it out!

@alicewriteswrongs
Copy link
Contributor Author

( I didn't quite get the type checker to pass completely. You can run the check with docker-compose run watch npm run-script flow)

@Ferdi
Copy link
Contributor

Ferdi commented Jun 10, 2016

@noisecapella
Copy link
Contributor

I'm getting this error, did you find a workaround? libelf.so.1: cannot open shared object file: No such file or directory

@noisecapella
Copy link
Contributor

Found this to fix it: flow/flow-bin#26
We can add the command to Dockerfile-node

@alicewriteswrongs
Copy link
Contributor Author

Oh yeah, I added that locally but didn't push it to github yet.
On Jun 10, 2016 5:55 PM, "George Schneeloch" [email protected]
wrote:

Found this to fix it: flow/flow-bin#26
flow/flow-bin#26
We can add the command to Dockerfile-node


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#522 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AF64nFvSf06RIjV1TWxaBqKMj9PuWWsXks5qKd03gaJpZM4IzDM-
.

@alicewriteswrongs
Copy link
Contributor Author

Just pushed the updated code - the docker issue should be sorted, and I also added a declarations.js file (where we can declare the type of SETTINGS and anything else which isn't initialized in the type-checked code).

@noisecapella
Copy link
Contributor

I tried to use flow comments to fix the flow warnings but I couldn't get it to work properly

@alicewriteswrongs
Copy link
Contributor Author

which ones were you trying to fix? The branch on github (ap/flow-js) should have a couple of things not passing still (I fiddled with it a little more this morning)

@noisecapella
Copy link
Contributor

entry.id, It keeps thinking the type is mixed even though I'm trying to set it specifically.

@alicewriteswrongs
Copy link
Contributor Author

there's an error in the checkFieldPresence function in validation.js and another in the renderWorkHistory method on EducationDialog I haven't handled

@alicewriteswrongs
Copy link
Contributor Author

oh yeah, that one

@alicewriteswrongs
Copy link
Contributor Author

are you setting it to type WorkHistoryEntry?

@alicewriteswrongs
Copy link
Contributor Author

fixed it! the most recent commit on the branch just has that change.

@alicewriteswrongs
Copy link
Contributor Author

alicewriteswrongs commented Jun 13, 2016

@alicewriteswrongs
Copy link
Contributor Author

@noisecapella what do you think about opening a PR for this?

@noisecapella
Copy link
Contributor

Sure, I feel like flow is missing some important things like annotating destructured props but I don't see the harm in trying it out

@alicewriteswrongs
Copy link
Contributor Author

Props should be fine - flow can use the propTypes declarations as types. The destructuring support is a little unfortunate though. It seems based on a couple of issues I read that they're actively working on it.

I was thinking we could merge it but not yet add it to the CI while we're still evaluating it, what do you think?

@noisecapella
Copy link
Contributor

Sounds good to me

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 a pull request may close this issue.

4 participants