More gen 22: Flow coverage exploration #9575

chrisnojima · 2017-11-16T03:35:26Z

Did some analysis of the flow coverage and found some underlying issues

Fix flow-types Promise
Flow-types usage of Buffer pulls in 3rd party buffer (for RN) but has no real types so it falls back to any. This caused a TON of issues. Instead we pull in the Node buffer and that reveals a bunch of stuff.

Chat

Fixed a bunch of mismatch types of string/buffer re: outboxid and messageid, convID vs conversationIDKey

Search/Teams:
Split the constants/types to remove cyclic dependencies in flow (if you run flow check --profile it'll tell you when it detects one)

chrisnojima · 2017-11-16T04:03:04Z

shared/actions/app-gen.js

@@ -17,7 +17,7 @@ export const mobileAppState = 'app:mobileAppState'
 export const createChangedActive = (payload: {|+userActive: boolean|}) => ({error: false, payload, type: changedActive})
 export const createChangedFocus = (payload: {|+appFocused: boolean|}) => ({error: false, payload, type: changedFocus})
 export const createLink = (payload: {|+link: string|}) => ({error: false, payload, type: link})
-export const createMobileAppState = (payload: {|+nextAppState: string|}) => ({error: false, payload, type: mobileAppState})
+export const createMobileAppState = (payload: {|+nextAppState: 'active' | 'background' | 'inactive'|}) => ({error: false, payload, type: mobileAppState})


from RN docs

chrisnojima · 2017-11-16T04:03:21Z

shared/actions/chat-gen.js

@@ -151,7 +151,7 @@ export const createSaveAttachment = (payload: {|+messageKey: Types.MessageKey|})
 export const createSaveAttachmentNative = (payload: {|+messageKey: Types.MessageKey|}) => ({error: false, payload, type: saveAttachmentNative})
 export const createSelectAttachment = (payload: {|+input: Types.AttachmentInput|}) => ({error: false, payload, type: selectAttachment})
 export const createSelectConversation = (payload: {|+conversationIDKey: ?Types.ConversationIDKey, +fromUser?: boolean|}) => ({error: false, payload, type: selectConversation})
-export const createSelectNext = (payload: {|+rows: Array<any>, +direction: -1 | 1|}) => ({error: false, payload, type: selectNext})
+export const createSelectNext = (payload: {|+rows: Array<{conversationIDKey: Types.ConversationIDKey}>, +direction: -1 | 1|}) => ({error: false, payload, type: selectNext})


really we care about pulling the convoId out

chrisnojima · 2017-11-16T04:03:32Z

shared/actions/chat/inbox.js

@@ -40,6 +40,7 @@ const _getInboxQuery = {
 // Update inboxes that have been reset
 function* _updateFinalized(inbox: RPCChatTypes.UnverifiedInboxUIItems): Generator<any, void, any> {
  const finalizedState: Types.FinalizedState = I.Map(
+    // TODO i *think* this typing is totally incorrect. should be .items etc


this is wrong

chrisnojima · 2017-11-16T04:05:59Z

shared/actions/chat/inbox.js

@@ -387,7 +392,7 @@ function* _chatInboxFailedSubSaga(params) {
      ? I.List([].concat(error.rekeyInfo.writerNames, error.rekeyInfo.readerNames).filter(Boolean))
      : I.List(error.unverifiedTLFName.split(',')),
    status: 'unfiled',
-    time: error.remoteConv.readerInfo.mtime,
+    time: error.remoteConv.readerInfo ? error.remoteConv.readerInfo.mtime : 0,


avdl says this can be missing

chrisnojima · 2017-11-16T04:07:03Z

shared/actions/chat/inbox.js

@@ -650,7 +655,7 @@ function* _sendNotifications(action: ChatGen.AppendMessagesPayload): Saga.SagaGe
      if (message && message.type === 'Text') {
        console.log('Sending Chat notification')
        const snippet = Constants.makeSnippet(Constants.serverMessageToMessageText(message))
-        yield Saga.put((dispatch: Dispatch) => {


we use Dispatch globally a lot but its really untyped. The one from redux also doesn't understand thunks

chrisnojima · 2017-11-16T04:07:37Z

shared/actions/chat/send-messages.js

@@ -42,9 +42,12 @@ function* deleteMessage(action: ChatGen.DeleteMessagePayload): SagaGenerator<any
    }
    const tlfName: string = inboxConvo.name

+    const parsedMessageID = lastMessageID ? Constants.parseMessageID(lastMessageID) : null
+    const clientPrev = parsedMessageID && parsedMessageID.type === 'rpcMessageID' ? parsedMessageID.msgID : 0


the only valid clientPrevs are from real messages and not the other things in the stream

chrisnojima · 2017-11-16T04:08:00Z

shared/actions/chat/thread-content.js

@@ -386,7 +386,7 @@ function _unboxedToMessage(
      : null
    // $FlowIssue
    const messageText: ChatTypes.MessageText = message.outbox.body
-    const outboxIDKey = payload.outboxID && Constants.outboxIDToKey(payload.outboxID)


outboxIDKeys are already coming down as the hex strings

chrisnojima · 2017-11-16T04:08:31Z

shared/engine/types.js.flow

@@ -1,6 +1,5 @@
 // @flow
 import {RPCError} from '../util/errors'
-import {Buffer} from 'buffer'


we caused the confusion w/ Buffer

chrisnojima · 2017-11-16T15:14:25Z

shared/actions/chat/inbox.js

-      totals[conversationIDKey] = total
-      if (badged) {
-        badges[conversationIDKey] = badged
+      const convID = conv.get('convID')


likely never null in practice but is possible

chrisnojima · 2017-11-16T15:14:57Z

shared/actions/git.js

@@ -156,7 +156,7 @@ const _onTabChange = (action: RouteTreeTypes.SwitchTo) => {
 }

 function* _handleIncomingGregor(action: GitGen.HandleIncomingGregorPayload): Saga.SagaGenerator<any, any> {
-  const msgs = action.payload.messages.map(msg => JSON.parse(msg.body))
+  const msgs = action.payload.messages.map(msg => JSON.parse(msg.body.toString()))


I think this worked before because the buffer is essentially casting to a string implicitly using toString() but this thing really is a buffer

chrisnojima · 2017-11-16T15:15:26Z

shared/actions/gregor.js

-  const createdTLFs = msgsWithParsedBodies.filter(m => m.body.action === 'create')
-
-  const username: string = (yield Saga.select(usernameSelector): any)
+  const createdTLFs: Array<{action: string, tlf: ?string}> = kbfsFavoriteMessages


flow got confused when we overwrote the body prop with a new body. we don't even use the raw ...m part so lets just get rid of it

chrisnojima · 2017-11-16T15:15:41Z

shared/actions/teams/index.js

@@ -371,9 +371,8 @@ function _afterGetChannels(

  const convs = results.convs || []
  convs.forEach(conv => {
-    const convID = ChatConstants.conversationIDToKey(conv.convID)


conv.convID is actually a string already

chrisnojima · 2017-11-16T15:16:00Z

shared/actions/teams/index.js

-        ChatConstants.keyToConversationID(channelnameToConvID[channelname]),
-    })
-  })
+    const convID =


rpc needs this to be non null

chrisnojima · 2017-11-16T15:16:20Z

shared/actions/teams/index.js

+        convID,
+      })
+    }
+  }).filter(Boolean)


filtering this cause we can return from map w/ undefined

chrisnojima · 2017-11-16T16:01:47Z

shared/chat/inbox/container.js

@@ -26,7 +27,9 @@ const getIsEmpty = (state: TypedState) => state.chat.get('inboxIsEmpty')
 const getPending = (state: TypedState) => state.chat.get('pendingConversations')
 const getSmallTimestamps = (state: TypedState) => state.chat.getIn(['inboxSmallTimestamps'], I.Map())
 const getSupersededBy = (state: TypedState) => state.chat.get('inboxSupersededBy')
-const _rowsForSelect = (rows: Array<any>) => rows.filter(r => ['small', 'big'].includes(r.type))
+const _rowsForSelect = (rows: Array<Inbox.RowItem>): Array<Inbox.RowItemSmall | Inbox.RowItemBig> =>
+  // $FlowIssue doesn't underestand filter refinement sadly


facebook/flow#1414

chrisnojima · 2017-11-16T16:02:23Z

shared/chat/inbox/container.js

  }
 }

-const mapDispatchToProps = (dispatch: Dispatch, {focusFilter, routeState, setRouteState}) => ({
+const mapDispatchToProps = (dispatch: any => void, {focusFilter, routeState, setRouteState}: OwnProps) => ({


TODO (later) we could update our dispatch type

chrisnojima · 2017-11-16T19:58:48Z

shared/constants/search.js

@@ -114,54 +59,18 @@ function maybeUpgradeSearchResultIdToKeybaseId(
  return id
 }

-function platformToIcon(service: Service): IconType {


these are never used

chrisnojima · 2017-11-16T19:59:24Z

shared/engine/index.js.flow

@@ -0,0 +1,37 @@
+// @flow
+// Sep. engine file so we can avoid import cycles w/ flow-types.js


flow-types pulls in engine which pulls in flow-types etc, lets not have that happen

chrisnojima · 2017-11-16T20:00:09Z

@keybase/react-hackers this is ready

chrisnojima · 2017-11-26T00:10:12Z

I merged 21 and below so this needs to be reopened vs master

WIP WIP WIP WIP WIP WIP remove flow cyclic deps. split teams / search consts add coverage to packagejson

chrisnojima · 2017-11-27T00:43:28Z

replaced by #9685

chrisnojima commented Nov 16, 2017

View reviewed changes

chrisnojima changed the title ~~WIP: Flow coverage exploration~~ More gen 22: Flow coverage exploration Nov 16, 2017

chrisnojima commented Nov 16, 2017

View reviewed changes

WIP

4aff76f

WIP WIP WIP WIP WIP WIP remove flow cyclic deps. split teams / search consts add coverage to packagejson

chrisnojima force-pushed the nojima/DESKTOP-more-gen-2-22-2 branch from 99ae001 to 4aff76f Compare November 26, 2017 23:51

chrisnojima mentioned this pull request Nov 27, 2017

More gen 22: Flow coverage exploration (remade) #9685

Merged

chrisnojima closed this Nov 27, 2017

chrisnojima deleted the nojima/DESKTOP-more-gen-2-22-2 branch March 14, 2018 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More gen 22: Flow coverage exploration #9575

More gen 22: Flow coverage exploration #9575

chrisnojima commented Nov 16, 2017 •

edited

Loading

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017 •

edited

Loading

chrisnojima Nov 16, 2017

chrisnojima Nov 16, 2017

chrisnojima commented Nov 16, 2017

chrisnojima commented Nov 26, 2017

chrisnojima commented Nov 27, 2017

		@@ -0,0 +1,37 @@
		// @flow
		// Sep. engine file so we can avoid import cycles w/ flow-types.js

More gen 22: Flow coverage exploration #9575

More gen 22: Flow coverage exploration #9575

Conversation

chrisnojima commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisnojima Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisnojima commented Nov 16, 2017

chrisnojima commented Nov 26, 2017

chrisnojima commented Nov 27, 2017

chrisnojima commented Nov 16, 2017 •

edited

Loading

chrisnojima Nov 16, 2017 •

edited

Loading