Added Flow Typing #778

Merged
liamcardenas merged 9 commits from flowtype into master 2017-12-01 01:03:47 +01:00
2 changed files with 4 additions and 2 deletions
Showing only changes of commit 1fb55c4a93 - Show all commits

View file

@ -10,5 +10,6 @@
[options] [options]
suppress_comment=\\(.\\|\n\\)*\\$FlowFixMe suppress_comment=\\(.\\|\n\\)*\\$FlowFixMe
suppress_comment=\\(.\\|\n\\)*\\$FlowIssue suppress_comment=\\(.\\|\n\\)*\\$FlowIssue
module.name_mapper='^constants\(.*\)$' -> '<PROJECT_ROOT>/js/constants\1'
[strict] [strict]

View file

@ -1,7 +1,7 @@
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
// @flow // @flow
import * as types from "../../constants/action_types"; import * as types from "constants/action_types";
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
import * as modalTypes from "../../constants/modal_types"; import * as modalTypes from "constants/modal_types";
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
// $FlowIssue: Flow cannot find election // $FlowIssue: Flow cannot find election
const { remote } = require("electron"); const { remote } = require("electron");
@ -48,6 +48,7 @@ const defaultState: AppState = {
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
hasSignature: false, hasSignature: false,
badgeNumber: 0, badgeNumber: 0,
volume: Number(sessionStorage.getItem("volume")) || 1, volume: Number(sessionStorage.getItem("volume")) || 1,
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
downloadProgress: undefined, downloadProgress: undefined,
upgradeDownloading: undefined, upgradeDownloading: undefined,
upgradeDownloadComplete: undefined, upgradeDownloadComplete: undefined,

kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/
kauffj commented 2017-11-29 23:53:40 +01:00 (Migrated from github.com)
Review

thoughts on all upper case for types?

thoughts on all upper case for types?
kauffj commented 2017-11-29 23:54:20 +01:00 (Migrated from github.com)
Review

Any particular reason are these optional properties that we don't set on defaultState? IMO optional properties should generally be avoided.

Any particular reason are these optional properties that we don't set on `defaultState`? IMO optional properties should generally be avoided.
kauffj commented 2017-11-29 23:54:53 +01:00 (Migrated from github.com)
Review

(also, state types will generally need to be exported so they can be used by selectors)

(also, state types will generally need to be exported so they can be used by selectors)
kauffj commented 2017-11-29 23:59:13 +01:00 (Migrated from github.com)
Review

Isn't this just ?string

Isn't this just `?string`
kauffj commented 2017-11-30 00:00:50 +01:00 (Migrated from github.com)
Review

Isn't this an unsealed Object or null? (not clear on Flow syntax for this: ?{}?)

Isn't this an unsealed `Object` or `null`? (not clear on Flow syntax for this: `?{}`?)
kauffj commented 2017-11-30 00:02:11 +01:00 (Migrated from github.com)
Review

If this is the return value for an interval call, I believe it is ?number

If this is the return value for an interval call, I believe it is `?number`
kauffj commented 2017-11-30 00:02:58 +01:00 (Migrated from github.com)
Review

I don't remember what's put in this but I'm just hating on all of your usage of mixed at this point.

I don't remember what's put in this but I'm just hating on all of your usage of `mixed` at this point.
liamcardenas commented 2017-11-30 00:05:45 +01:00 (Migrated from github.com)
Review

I believe mixed is the correct way for to indicate Object

I believe mixed is the correct way for to indicate Object
kauffj commented 2017-11-30 00:06:02 +01:00 (Migrated from github.com)
Review

Also also, this should be a sealed object.

Also also, this should be a sealed object.
liamcardenas commented 2017-11-30 00:08:24 +01:00 (Migrated from github.com)
Review

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though.

The one edge case I could see, and I will double check this, is, as I was saying in slack:

downloadProgress?: number could be a number or undefined
downloadProgress: ?number could be a number or undefined or null

In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.

optional properties can be thought of as implicitly there, but set to "undefined". I am more than happy to add them, though. The one edge case I could see, and I will double check this, is, as I was saying in slack: downloadProgress?: number could be a number or undefined downloadProgress: ?number could be a number or undefined or null In order to remove optional fields, we would have to either allow null or determine a non-null default value for each field.
liamcardenas commented 2017-11-30 00:09:13 +01:00 (Migrated from github.com)
Review

me too, i'll figure it out

me too, i'll figure it out
liamcardenas commented 2017-11-30 00:10:31 +01:00 (Migrated from github.com)
Review

I agree, I meant to bring this up, we also need to figure out how to properly do naming.

I agree, I meant to bring this up, we also need to figure out how to properly do naming.
liamcardenas commented 2017-11-30 00:13:19 +01:00 (Migrated from github.com)
Review

ah i didn't realize that, thanks.

ah i didn't realize that, thanks.
liamcardenas commented 2017-11-30 00:14:56 +01:00 (Migrated from github.com)
Review

yes, thanks

yes, thanks
liamcardenas commented 2017-11-30 08:00:34 +01:00 (Migrated from github.com)
Review

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt

I am going to go through each of these one by one to determine whether or not I can add null, or some other initial value to them instead of undefined, however, there inevitably will be some fields that are initialized as undefined (given what we decided in terms of having no optional types + using undefined as a distinct value). Does this seem alright to you guys? @kauffj @seanyesmunt
kauffj commented 2017-11-30 17:49:57 +01:00 (Migrated from github.com)
Review

Yes, given that the existing behavior a property not explicitly set on defaultState is to default to undefined, it makes sense to set values to undefined rather than null to match existing behavior.

Yes, given that the existing behavior a property not explicitly set on `defaultState` is to default to `undefined`, it makes sense to set values to `undefined` rather than `null` to match existing behavior.
liamcardenas commented 2017-11-30 18:05:54 +01:00 (Migrated from github.com)
Review

Got it. Makes sense. I'm cool with this.

Got it. Makes sense. I'm cool with this.
kauffj commented 2017-11-30 23:32:42 +01:00 (Migrated from github.com)
Review

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/

@liamcardenas I do not think that is correct https://flow.org/en/docs/types/mixed/