Flow #325

Merged
merged 6 commits into from Jul 14, 2016

Conversation

Projects
None yet
2 participants
@jlongster
Contributor

jlongster commented Jul 13, 2016

I'm so sorry for the size of this PR.

Flow makes us use babel to strip the types, and that tends to have a cascading effect. I'm not sure if we'll be able to avoid babel in the long run though. The immediate thing that comes to mind is JSX. If we are ok with a build step (which we are), we'll probably end up using that.

The main annoyance was that the linter was complaining now because the "use strict" directive is redundant; all files are passed through babel which forces strict mode. So the majority of the files changes is just removing all strict directives.

The interesting parts are the files that I annotated with flow:

./public/js/actions/breakpoints.js
./public/js/actions/types.js
./public/js/reducers/sources.js
./public/js/reducers/tests/sources.js
./public/js/selectors.js
./public/js/util/fromJS.js
./public/js/util/makeRecord.js
./public/js/util/test-head.js

I wanted to have one of each type of file annotated: a reducer, an action creator, the selectors, and a test. For example, if you go into selectors.js, in getSources, and introduce a typo like return state.sources.sources2, you will see a type error when you run flow (or npm run flow). There are several places in the above files where this will happen now.

I went through many attempts at the initial integration, so some things might still be leftover from that. There are still several places in these files that aren't type-checked, and we need to figure out how to check everything in them, but this PR is already large enough.

Another example is going into public/js/reducers/tests/sources.js and changing the action type to a typo like ADD_SOURCE2. You'll see that even the action objects are fully type-checked; they need to conform to a specific action type specified in actions/types.js.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jul 13, 2016

Contributor

The hardest part about this was trying to type immutable.js, which is certainly possible but required me to learn enough flow before I could get something working. You'll see makeRecord.js which is only partially used, which will fully type immutable records. We can't use it fully until 0.29 comes out with a serious perf fix (should come out this week). Right now, the state in reducers/sources.js isn't type-checked much because the Record type returns any from it's methods.

It also encourages us to use records so that we can type check it. Switching the sources state to be a record introduced other problems, like not being able to use a naive fromJS anymore (which we keep running into). I wrote a dehydrate function which explicitly knows how to dehydrate a JSON-based app state into our immutable one. We were already using our own fromJS before anyway. There is probably a better long-term solution for this.

Contributor

jlongster commented Jul 13, 2016

The hardest part about this was trying to type immutable.js, which is certainly possible but required me to learn enough flow before I could get something working. You'll see makeRecord.js which is only partially used, which will fully type immutable records. We can't use it fully until 0.29 comes out with a serious perf fix (should come out this week). Right now, the state in reducers/sources.js isn't type-checked much because the Record type returns any from it's methods.

It also encourages us to use records so that we can type check it. Switching the sources state to be a record introduced other problems, like not being able to use a naive fromJS anymore (which we keep running into). I wrote a dehydrate function which explicitly knows how to dehydrate a JSON-based app state into our immutable one. We were already using our own fromJS before anyway. There is probably a better long-term solution for this.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jul 13, 2016

Contributor

karma and cypress are still broken, and I bet storybook is too (all that storybook should need is to use dehydrate instead of fromJS.

Contributor

jlongster commented Jul 13, 2016

karma and cypress are still broken, and I bet storybook is too (all that storybook should need is to use dehydrate instead of fromJS.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jul 13, 2016

Contributor

There are 2 commits and if you look at the first one you'll see the most interesting bits. The second one fixes all the linting issues and other nonsense.

Contributor

jlongster commented Jul 13, 2016

There are 2 commits and if you look at the first one you'll see the most interesting bits. The second one fixes all the linting issues and other nonsense.

public/js/test/integration/fixtures.js
@@ -1,6 +1,6 @@
// turn theses tests on when you want to write new fixture data
// you also need to turn on the cypress-server to be able to save the fixtures.
-xdescribe("Fixtures", function() {
+describe("Fixtures", function() {

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

re-add xdescribe and cypress should pass.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

re-add xdescribe and cypress should pass.

This comment has been minimized.

@jlongster

jlongster Jul 13, 2016

Contributor

Oh! Hah I thought I had added a typo.

@jlongster

jlongster Jul 13, 2016

Contributor

Oh! Hah I thought I had added a typo.

@jasonLaster

This comment has been minimized.

Show comment
Hide comment
@jasonLaster

jasonLaster Jul 13, 2016

Contributor

it looks like karma has some functions like getSources which are undefined. Is that related to dehydrate and/or selectors?

Contributor

jasonLaster commented Jul 13, 2016

it looks like karma has some functions like getSources which are undefined. Is that related to dehydrate and/or selectors?

public/js/actions/breakpoints.js
const constants = require("../constants");
const { PROMISE } = require("../util/redux/middleware/promise");
const { getBreakpoint, getBreakpoints } = require("../selectors");
const { Task } = require("../util/task");
const fromJS = require("../util/fromJS");
-function enableBreakpoint(location) {
+import type { Location } from "./types";

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

Is it possible to use commonjs here? or is there something about types that means we need to use import?

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

Is it possible to use commonjs here? or is there something about types that means we need to use import?

This comment has been minimized.

@jlongster

jlongster Jul 13, 2016

Contributor

Nope, the syntax for types is only ES6 :/ But that's because CommonJS doesn't really have a way to do this. You'd have to special syntax for CommonJS, which starts getting weird because usually require is just an expression. So I get their design choice.

ES6 is the future anyway. Since we're using a build step, I wouldn't be surprised if we converted to ES6 modules at some point. But I don't think mixing the styles is too bad.

@jlongster

jlongster Jul 13, 2016

Contributor

Nope, the syntax for types is only ES6 :/ But that's because CommonJS doesn't really have a way to do this. You'd have to special syntax for CommonJS, which starts getting weird because usually require is just an expression. So I get their design choice.

ES6 is the future anyway. Since we're using a build step, I wouldn't be surprised if we converted to ES6 modules at some point. But I don't think mixing the styles is too bad.

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

makes sense.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

makes sense.

public/js/actions/breakpoints.js
+
+function addBreakpoint(
+ location: Location,
+ { condition, getTextForLine } : AddBreakpointOpts = {}

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

Maybe it's because I'm new to this, but AddBreakpointOpts seems unhelpful and adds a lot of clutter

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

Maybe it's because I'm new to this, but AddBreakpointOpts seems unhelpful and adds a lot of clutter

This comment has been minimized.

@jlongster

jlongster Jul 13, 2016

Contributor

It definitely does. I just wanted to get this PR out. It's probably because I'm new to flow that this is verbose. We'll figure out the right balance over time, and I'll look at this again before merging.

@jlongster

jlongster Jul 13, 2016

Contributor

It definitely does. I just wanted to get this PR out. It's probably because I'm new to flow that this is verbose. We'll figure out the right balance over time, and I'll look at this again before merging.

public/js/actions/breakpoints.js
@@ -37,10 +53,11 @@ function addBreakpoint(location, { condition, getTextForLine } = {}) {
breakpoint: bp.toJS(),
condition: condition,
[PROMISE]: Task.spawn(function* () {
- const { id, actualLocation } = yield client.setBreakpoint(
+ const res: any = yield client.setBreakpoint(

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

This also seems unhelpful. It might be nice to type the client, but on the other hand I also don't expect it to change much and I appreciate being able to destructure on assignments.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

This also seems unhelpful. It might be nice to type the client, but on the other hand I also don't expect it to change much and I appreciate being able to destructure on assignments.

This comment has been minimized.

@jlongster

jlongster Jul 13, 2016

Contributor

Destructuring is supported, I just ran into an issue here and I can't remember what it was. I didn't spend much time on this part of the file, just had to get something working.

@jlongster

jlongster Jul 13, 2016

Contributor

Destructuring is supported, I just ran into an issue here and I can't remember what it was. I didn't spend much time on this part of the file, just had to get something working.

breakpoints, eventListeners, sources, tabs, pause
-);
+) : typeof breakpoints);

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

what does typeof breakpoints do?

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

what does typeof breakpoints do?

This comment has been minimized.

@jlongster

jlongster Jul 13, 2016

Contributor

It statically gives the type system whatever type breakpoints is. This tells the system that what I'm returning has the exact structure of the breakpoints object, so it lets you do this dynamic Object.assign munging but still type it.

@jlongster

jlongster Jul 13, 2016

Contributor

It statically gives the type system whatever type breakpoints is. This tells the system that what I'm returning has the exact structure of the breakpoints object, so it lets you do this dynamic Object.assign munging but still type it.

- Task.spawn(function* () {
- yield loadBadSource(store);
- const fooSourceText = getSourceText(store.getState(), "foo1");
+ const fooSourceText = getSourceText(store.getState(), "badId");

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

nice

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

nice

@@ -0,0 +1,38 @@
+// @flow
+
+export type AsyncStatus = "start" | "done" | "error";

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

nice

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

nice

+ name: string
+): (init: $Shape<T>) => Record<T> {
+ return I.Record(spec, name);
+}

This comment has been minimized.

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

cool

@jasonLaster

jasonLaster Jul 13, 2016

Contributor

cool

@jasonLaster

This comment has been minimized.

Show comment
Hide comment
@jasonLaster

jasonLaster Jul 13, 2016

Contributor

Overall, this looks great. A couple small style nits. I'd prefer and start small and use the features that are clearly helpful. There were some examples where flow added annotations that were verbose, and it was hard to see the benefits being worthwhile.

I'd like to merge this soon, but I also don't want to add it if we break karma and cypress. It looks like the cypress fix is easy though. I'm looking into karma now on master so i'll have a better idea where the flakiness is coming from.

Contributor

jasonLaster commented Jul 13, 2016

Overall, this looks great. A couple small style nits. I'd prefer and start small and use the features that are clearly helpful. There were some examples where flow added annotations that were verbose, and it was hard to see the benefits being worthwhile.

I'd like to merge this soon, but I also don't want to add it if we break karma and cypress. It looks like the cypress fix is easy though. I'm looking into karma now on master so i'll have a better idea where the flakiness is coming from.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jul 13, 2016

Contributor

it looks like karma has some functions like getSources which are undefined. Is that related to dehydrate and/or selectors?

I'm not sure, but I bet it's my fault, I don't think I saw those errors in the previous flaky errors. I will look into those.

Contributor

jlongster commented Jul 13, 2016

it looks like karma has some functions like getSources which are undefined. Is that related to dehydrate and/or selectors?

I'm not sure, but I bet it's my fault, I don't think I saw those errors in the previous flaky errors. I will look into those.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jul 13, 2016

Contributor

Overall, this looks great. A couple small style nits. I'd prefer and start small and use the features that are clearly helpful. There were some examples where flow added annotations that were verbose, and it was hard to see the benefits being worthwhile.

Yeah, we'll figure out a balance over time. I think it's a good goal to type check as much as possible. The problem with holes is that they tend to "bleed out", but there are places where it's OK. The worst parts here was in parts of breakpoints.js where I did things quickly and didn't think much of it. There are probably more elegant solutions.

I'd like to merge this soon, but I also don't want to add it if we break karma and cypress. It looks like the cypress fix is easy though. I'm looking into karma now on master so i'll have a better idea where the flakiness is coming from.

Yeah, let's not merge it until I fix it up a little more. I'll look into karma too because I'm pretty sure this error is my fault.

Contributor

jlongster commented Jul 13, 2016

Overall, this looks great. A couple small style nits. I'd prefer and start small and use the features that are clearly helpful. There were some examples where flow added annotations that were verbose, and it was hard to see the benefits being worthwhile.

Yeah, we'll figure out a balance over time. I think it's a good goal to type check as much as possible. The problem with holes is that they tend to "bleed out", but there are places where it's OK. The worst parts here was in parts of breakpoints.js where I did things quickly and didn't think much of it. There are probably more elegant solutions.

I'd like to merge this soon, but I also don't want to add it if we break karma and cypress. It looks like the cypress fix is easy though. I'm looking into karma now on master so i'll have a better idea where the flakiness is coming from.

Yeah, let's not merge it until I fix it up a little more. I'll look into karma too because I'm pretty sure this error is my fault.

@clarkbw clarkbw referenced this pull request Jul 14, 2016

Closed

Use Flow to manage types #308

@jlongster jlongster merged commit ccb58b9 into master Jul 14, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@clarkbw clarkbw deleted the flow branch Aug 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment