From 64b06ba6f6c29d90ed524e2d2d4addb27ce13623 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 20 Feb 2020 12:54:31 -0800 Subject: [PATCH] Fixed some issues with too many devices showing up in the sidebar Summary: Device management was inconsistent so far, this diff addresses the following issues * pending a subtle timing issue, a physical android device might also show up as emulator, so effectively the device would be shown twice, but with the same content * Metro devices now behave more like the android devices: offline devices are replaced if it comes online again * Generalized this logic; the reducer now forces serials to be unique * Fixed issue where a Metro device that disconnected due to a connection failure would be archived twice * Use the metro connection url as serial, to have a slightly more future proof serial Reviewed By: jknoxville Differential Revision: D19996385 fbshipit-source-id: 0f6e3ddc6444542553d25cc3b592591652d688f2 --- src/devices/MetroDevice.tsx | 2 +- src/dispatcher/metroDevice.tsx | 23 +++++----- src/reducers/__tests__/connections.node.tsx | 50 +++++++++++++++++++++ src/reducers/connections.tsx | 15 ++++++- 4 files changed, 78 insertions(+), 12 deletions(-) diff --git a/src/devices/MetroDevice.tsx b/src/devices/MetroDevice.tsx index d038fc1c2..e1a38969e 100644 --- a/src/devices/MetroDevice.tsx +++ b/src/devices/MetroDevice.tsx @@ -201,7 +201,7 @@ export default class MetroDevice extends BaseDevice { archive() { return new ArchivedDevice( - this.serial + v4(), + this.serial, this.deviceType, this.title, this.os, diff --git a/src/dispatcher/metroDevice.tsx b/src/dispatcher/metroDevice.tsx index 433004012..22e52972a 100644 --- a/src/dispatcher/metroDevice.tsx +++ b/src/dispatcher/metroDevice.tsx @@ -20,7 +20,6 @@ const METRO_URL = `http://${METRO_HOST}:${METRO_PORT}`; const METRO_LOGS_ENDPOINT = `ws://${METRO_HOST}:${METRO_PORT}/events`; const METRO_MESSAGE = ['React Native packager is running', 'Metro is running']; const QUERY_INTERVAL = 5000; -const METRO_DEVICE_ID = 'metro'; // there is always only one activve async function isMetroRunning(): Promise { return new Promise(resolve => { @@ -50,7 +49,7 @@ async function registerDevice( store: Store, logger: Logger, ) { - const metroDevice = new MetroDevice(METRO_DEVICE_ID, ws); + const metroDevice = new MetroDevice(METRO_URL, ws); logger.track('usage', 'register-device', { os: 'Metro', name: metroDevice.title, @@ -60,7 +59,7 @@ async function registerDevice( store.dispatch({ type: 'REGISTER_DEVICE', payload: metroDevice, - serial: METRO_DEVICE_ID, + serial: METRO_URL, }); registerDeviceCallbackOnPlugins( @@ -74,20 +73,20 @@ async function registerDevice( async function unregisterDevices(store: Store, logger: Logger) { logger.track('usage', 'unregister-device', { os: 'Metro', - serial: METRO_DEVICE_ID, + serial: METRO_URL, }); let archivedDevice: ArchivedDevice | undefined = undefined; const device = store .getState() - .connections.devices.find(device => device.serial === METRO_DEVICE_ID); + .connections.devices.find(device => device.serial === METRO_URL); if (device && !device.isArchived) { archivedDevice = device.archive(); } store.dispatch({ type: 'UNREGISTER_DEVICES', - payload: new Set([METRO_DEVICE_ID]), + payload: new Set([METRO_URL]), }); if (archivedDevice) { @@ -102,6 +101,7 @@ async function unregisterDevices(store: Store, logger: Logger) { export default (store: Store, logger: Logger) => { let timeoutHandle: NodeJS.Timeout; let ws: WebSocket | undefined; + let unregistered = false; async function tryConnectToMetro() { if (ws) { @@ -118,10 +118,13 @@ export default (store: Store, logger: Logger) => { }; _ws.onclose = _ws.onerror = () => { - clearTimeout(guard); - ws = undefined; - unregisterDevices(store, logger); - scheduleNext(); + if (!unregistered) { + unregistered = true; + clearTimeout(guard); + ws = undefined; + unregisterDevices(store, logger); + scheduleNext(); + } }; const guard = setTimeout(() => { diff --git a/src/reducers/__tests__/connections.node.tsx b/src/reducers/__tests__/connections.node.tsx index 563917642..3c2f1bff9 100644 --- a/src/reducers/__tests__/connections.node.tsx +++ b/src/reducers/__tests__/connections.node.tsx @@ -12,6 +12,7 @@ import {State, selectPlugin} from '../connections'; import BaseDevice from '../../devices/BaseDevice'; import MacDevice from '../../devices/MacDevice'; import {FlipperDevicePlugin} from '../../plugin'; +import MetroDevice from '../../devices/MetroDevice'; test('REGISTER_DEVICE doesnt remove error', () => { const initialState: State = reducer(undefined, { @@ -34,6 +35,55 @@ test('REGISTER_DEVICE doesnt remove error', () => { ]); }); +test('doing a double REGISTER_DEVICE keeps the last', () => { + const device1 = new BaseDevice('serial', 'physical', 'title', 'Android'); + const device2 = new BaseDevice('serial', 'physical', 'title2', 'Android'); + const initialState: State = reducer(undefined, { + type: 'REGISTER_DEVICE', + payload: device1, + }); + expect(initialState.devices.length).toBe(1); + expect(initialState.devices[0]).toBe(device1); + + const endState = reducer(initialState, { + type: 'REGISTER_DEVICE', + payload: device2, + }); + expect(endState.devices.length).toBe(1); + expect(endState.devices[0]).toBe(device2); +}); + +test('register, remove, re-register a metro device works correctly', () => { + const device1 = new MetroDevice('http://localhost:8081', undefined); + let state: State = reducer(undefined, { + type: 'REGISTER_DEVICE', + payload: device1, + }); + expect(state.devices.length).toBe(1); + expect(state.devices[0].displayTitle()).toBe('React Native'); + + const archived = device1.archive(); + state = reducer(state, { + type: 'UNREGISTER_DEVICES', + payload: new Set([device1.serial]), + }); + expect(state.devices.length).toBe(0); + + state = reducer(state, { + type: 'REGISTER_DEVICE', + payload: archived, + }); + expect(state.devices.length).toBe(1); + expect(state.devices[0].displayTitle()).toBe('React Native (Offline)'); + + state = reducer(state, { + type: 'REGISTER_DEVICE', + payload: new MetroDevice('http://localhost:8081', undefined), + }); + expect(state.devices.length).toBe(1); + expect(state.devices[0].displayTitle()).toBe('React Native'); +}); + test('triggering REGISTER_DEVICE before REGISTER_PLUGINS still registers device plugins', () => { class TestDevicePlugin extends FlipperDevicePlugin { static id = 'test'; diff --git a/src/reducers/connections.tsx b/src/reducers/connections.tsx index 69c04ca53..dbe77472d 100644 --- a/src/reducers/connections.tsx +++ b/src/reducers/connections.tsx @@ -204,9 +204,22 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => { case 'REGISTER_DEVICE': { const {payload} = action; + const newDevices = state.devices.slice(); + const existing = state.devices.findIndex( + device => device.serial === payload.serial, + ); + if (existing !== -1) { + console.debug( + `Got a new device instance for already existing serial ${payload.serial}`, + ); + newDevices[existing] = payload; + } else { + newDevices.push(payload); + } + return updateSelection({ ...state, - devices: state.devices.concat(payload), + devices: newDevices, }); }