From c46d5187c1ca9b0c6ae67422d581fa3b53c065d8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Jun 2020 09:24:05 +0200 Subject: [PATCH 1/3] Removed duplicated code when binding to mercure by checking if enabled first --- src/mercure/helpers/index.js | 5 ++--- src/mercure/reducers/mercureInfo.js | 8 ++++++++ src/short-urls/ShortUrlsList.js | 5 +---- src/short-urls/services/provideServices.js | 2 +- src/tags/TagsList.js | 7 ++----- src/tags/services/provideServices.js | 2 +- src/visits/ShortUrlVisits.js | 11 +--------- src/visits/TagVisits.js | 11 +--------- src/visits/services/provideServices.js | 4 ++-- test/mercure/helpers/index.test.js | 13 ++++++------ test/mercure/reducers/mercureInfo.test.js | 24 +++++++++++++++++++--- test/short-urls/ShortUrlsList.test.js | 2 -- test/tags/TagsList.test.js | 2 +- test/visits/ShortUrlVisits.test.js | 2 -- test/visits/TagVisits.test.js | 2 -- 15 files changed, 47 insertions(+), 53 deletions(-) diff --git a/src/mercure/helpers/index.js b/src/mercure/helpers/index.js index fd69ce08..0cdd367c 100644 --- a/src/mercure/helpers/index.js +++ b/src/mercure/helpers/index.js @@ -1,10 +1,9 @@ import { EventSourcePolyfill as EventSource } from 'event-source-polyfill'; -export const bindToMercureTopic = (mercureInfo, realTimeUpdates, topic, onMessage, onTokenExpired) => () => { - const { enabled } = realTimeUpdates; +export const bindToMercureTopic = (mercureInfo, topic, onMessage, onTokenExpired) => () => { const { mercureHubUrl, token, loading, error } = mercureInfo; - if (!enabled || loading || error) { + if (loading || error) { return undefined; } diff --git a/src/mercure/reducers/mercureInfo.js b/src/mercure/reducers/mercureInfo.js index e9f812d1..62a9b9db 100644 --- a/src/mercure/reducers/mercureInfo.js +++ b/src/mercure/reducers/mercureInfo.js @@ -29,8 +29,16 @@ export default handleActions({ export const loadMercureInfo = (buildShlinkApiClient) => () => async (dispatch, getState) => { dispatch({ type: GET_MERCURE_INFO_START }); + + const { settings } = getState(); const { mercureInfo } = buildShlinkApiClient(getState); + if (!settings.realTimeUpdates.enabled) { + dispatch({ type: GET_MERCURE_INFO_ERROR }); + + return; + } + try { const result = await mercureInfo(); diff --git a/src/short-urls/ShortUrlsList.js b/src/short-urls/ShortUrlsList.js index e0c2f30c..9df2250b 100644 --- a/src/short-urls/ShortUrlsList.js +++ b/src/short-urls/ShortUrlsList.js @@ -9,7 +9,6 @@ import SortingDropdown from '../utils/SortingDropdown'; import { determineOrderDir } from '../utils/utils'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; import { bindToMercureTopic } from '../mercure/helpers'; -import { SettingsType } from '../settings/reducers/settings'; import { shortUrlType } from './reducers/shortUrlsList'; import { shortUrlsListParamsType } from './reducers/shortUrlsListParams'; import './ShortUrlsList.scss'; @@ -34,7 +33,6 @@ const propTypes = { createNewVisit: PropTypes.func, loadMercureInfo: PropTypes.func, mercureInfo: MercureInfoType, - settings: SettingsType, }; // FIXME Replace with typescript: (ShortUrlsRow component) @@ -52,7 +50,6 @@ const ShortUrlsList = (ShortUrlsRow) => { createNewVisit, loadMercureInfo, mercureInfo, - settings: { realTimeUpdates }, }) => { const { orderBy } = shortUrlsListParams; const [ order, setOrder ] = useState({ @@ -120,7 +117,7 @@ const ShortUrlsList = (ShortUrlsRow) => { return resetShortUrlParams; }, []); useEffect( - bindToMercureTopic(mercureInfo, realTimeUpdates, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), + bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), [ mercureInfo ] ); diff --git a/src/short-urls/services/provideServices.js b/src/short-urls/services/provideServices.js index 82a70db8..46254639 100644 --- a/src/short-urls/services/provideServices.js +++ b/src/short-urls/services/provideServices.js @@ -31,7 +31,7 @@ const provideServices = (bottle, connect) => { bottle.serviceFactory('ShortUrlsList', ShortUrlsList, 'ShortUrlsRow'); bottle.decorator('ShortUrlsList', connect( - [ 'selectedServer', 'shortUrlsListParams', 'mercureInfo', 'settings' ], + [ 'selectedServer', 'shortUrlsListParams', 'mercureInfo' ], [ 'listShortUrls', 'resetShortUrlParams', 'createNewVisit', 'loadMercureInfo' ] )); diff --git a/src/tags/TagsList.js b/src/tags/TagsList.js index f0e70874..4fc72741 100644 --- a/src/tags/TagsList.js +++ b/src/tags/TagsList.js @@ -5,7 +5,6 @@ import Message from '../utils/Message'; import SearchField from '../utils/SearchField'; import { serverType } from '../servers/prop-types'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { SettingsType } from '../settings/reducers/settings'; import { bindToMercureTopic } from '../mercure/helpers'; import { TagsListType } from './reducers/tagsList'; @@ -20,21 +19,19 @@ const propTypes = { createNewVisit: PropTypes.func, loadMercureInfo: PropTypes.func, mercureInfo: MercureInfoType, - settings: SettingsType, }; const TagsList = (TagCard) => { const TagListComp = ( - { filterTags, forceListTags, tagsList, selectedServer, createNewVisit, loadMercureInfo, mercureInfo, settings } + { filterTags, forceListTags, tagsList, selectedServer, createNewVisit, loadMercureInfo, mercureInfo } ) => { - const { realTimeUpdates } = settings; const [ displayedTag, setDisplayedTag ] = useState(); useEffect(() => { forceListTags(); }, []); useEffect( - bindToMercureTopic(mercureInfo, realTimeUpdates, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), + bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), [ mercureInfo ] ); diff --git a/src/tags/services/provideServices.js b/src/tags/services/provideServices.js index 7917d068..f1bc835d 100644 --- a/src/tags/services/provideServices.js +++ b/src/tags/services/provideServices.js @@ -29,7 +29,7 @@ const provideServices = (bottle, connect) => { bottle.serviceFactory('TagsList', TagsList, 'TagCard'); bottle.decorator('TagsList', connect( - [ 'tagsList', 'selectedServer', 'mercureInfo', 'settings' ], + [ 'tagsList', 'selectedServer', 'mercureInfo' ], [ 'forceListTags', 'filterTags', 'createNewVisit', 'loadMercureInfo' ] )); diff --git a/src/visits/ShortUrlVisits.js b/src/visits/ShortUrlVisits.js index c3eac779..131586fd 100644 --- a/src/visits/ShortUrlVisits.js +++ b/src/visits/ShortUrlVisits.js @@ -3,7 +3,6 @@ import PropTypes from 'prop-types'; import qs from 'qs'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; import { bindToMercureTopic } from '../mercure/helpers'; -import { SettingsType } from '../settings/reducers/settings'; import { shortUrlVisitsType } from './reducers/shortUrlVisits'; import ShortUrlVisitsHeader from './ShortUrlVisitsHeader'; import { shortUrlDetailType } from './reducers/shortUrlDetail'; @@ -26,7 +25,6 @@ const propTypes = { createNewVisit: PropTypes.func, loadMercureInfo: PropTypes.func, mercureInfo: MercureInfoType, - settings: SettingsType, }; const ShortUrlVisits = (VisitsStats) => { @@ -42,7 +40,6 @@ const ShortUrlVisits = (VisitsStats) => { createNewVisit, loadMercureInfo, mercureInfo, - settings: { realTimeUpdates }, }) => { const { params } = match; const { shortCode } = params; @@ -55,13 +52,7 @@ const ShortUrlVisits = (VisitsStats) => { getShortUrlDetail(shortCode, domain); }, []); useEffect( - bindToMercureTopic( - mercureInfo, - realTimeUpdates, - `https://shlink.io/new-visit/${shortCode}`, - createNewVisit, - loadMercureInfo - ), + bindToMercureTopic(mercureInfo, `https://shlink.io/new-visit/${shortCode}`, createNewVisit, loadMercureInfo), [ mercureInfo ], ); diff --git a/src/visits/TagVisits.js b/src/visits/TagVisits.js index ae17aacd..c570edf0 100644 --- a/src/visits/TagVisits.js +++ b/src/visits/TagVisits.js @@ -1,7 +1,6 @@ import React, { useEffect } from 'react'; import PropTypes from 'prop-types'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { SettingsType } from '../settings/reducers/settings'; import { bindToMercureTopic } from '../mercure/helpers'; import { TagVisitsType } from './reducers/tagVisits'; import TagVisitsHeader from './TagVisitsHeader'; @@ -19,7 +18,6 @@ const propTypes = { createNewVisit: PropTypes.func, loadMercureInfo: PropTypes.func, mercureInfo: MercureInfoType, - settings: SettingsType, }; const TagVisits = (VisitsStats, colorGenerator) => { @@ -32,20 +30,13 @@ const TagVisits = (VisitsStats, colorGenerator) => { createNewVisit, loadMercureInfo, mercureInfo, - settings: { realTimeUpdates }, }) => { const { params } = match; const { tag } = params; const loadVisits = (dates) => getTagVisits(tag, dates); useEffect( - bindToMercureTopic( - mercureInfo, - realTimeUpdates, - 'https://shlink.io/new-visit', - createNewVisit, - loadMercureInfo - ), + bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), [ mercureInfo ], ); diff --git a/src/visits/services/provideServices.js b/src/visits/services/provideServices.js index f0701a05..18da7c20 100644 --- a/src/visits/services/provideServices.js +++ b/src/visits/services/provideServices.js @@ -16,12 +16,12 @@ const provideServices = (bottle, connect) => { bottle.serviceFactory('VisitsStats', VisitsStats, 'VisitsParser', 'OpenMapModalBtn'); bottle.serviceFactory('ShortUrlVisits', ShortUrlVisits, 'VisitsStats'); bottle.decorator('ShortUrlVisits', connect( - [ 'shortUrlVisits', 'shortUrlDetail', 'mercureInfo', 'settings' ], + [ 'shortUrlVisits', 'shortUrlDetail', 'mercureInfo' ], [ 'getShortUrlVisits', 'getShortUrlDetail', 'cancelGetShortUrlVisits', 'createNewVisit', 'loadMercureInfo' ] )); bottle.serviceFactory('TagVisits', TagVisits, 'VisitsStats', 'ColorGenerator'); bottle.decorator('TagVisits', connect( - [ 'tagVisits', 'mercureInfo', 'settings' ], + [ 'tagVisits', 'mercureInfo' ], [ 'getTagVisits', 'cancelGetTagVisits', 'createNewVisit', 'loadMercureInfo' ] )); diff --git a/test/mercure/helpers/index.test.js b/test/mercure/helpers/index.test.js index 3ee91a3c..1e6fd3df 100644 --- a/test/mercure/helpers/index.test.js +++ b/test/mercure/helpers/index.test.js @@ -11,12 +11,11 @@ describe('helpers', () => { const onTokenExpired = jest.fn(); it.each([ - [{ loading: true, error: false }, { enabled: true }], - [{ loading: false, error: true }, { enabled: true }], - [{ loading: true, error: true }, { enabled: true }], - [{ loading: false, error: false }, { enabled: false }], - ])('does not bind an EventSource when disabled, loading or error', (mercureInfo, realTimeUpdates) => { - bindToMercureTopic(mercureInfo, realTimeUpdates)(); + [{ loading: true, error: false }], + [{ loading: false, error: true }], + [{ loading: true, error: true }], + ])('does not bind an EventSource when loading or error', (mercureInfo) => { + bindToMercureTopic(mercureInfo)(); expect(EventSource).not.toHaveBeenCalled(); expect(onMessage).not.toHaveBeenCalled(); @@ -36,7 +35,7 @@ describe('helpers', () => { error: false, mercureHubUrl, token, - }, { enabled: true }, topic, onMessage, onTokenExpired)(); + }, topic, onMessage, onTokenExpired)(); expect(EventSource).toHaveBeenCalledWith(hubUrl, { headers: { diff --git a/test/mercure/reducers/mercureInfo.test.js b/test/mercure/reducers/mercureInfo.test.js index fa93636e..c5219548 100644 --- a/test/mercure/reducers/mercureInfo.test.js +++ b/test/mercure/reducers/mercureInfo.test.js @@ -40,14 +40,31 @@ describe('mercureInfoReducer', () => { mercureInfo: jest.fn(() => result), }); const dispatch = jest.fn(); - const getState = () => ({}); + const createGetStateMock = (enabled) => jest.fn(() => ({ + settings: { + realTimeUpdates: { enabled }, + }, + })); afterEach(jest.resetAllMocks); + it('dispatches error when real time updates are disabled', async () => { + const apiClientMock = createApiClientMock(Promise.resolve(mercureInfo)); + const getState = createGetStateMock(false); + + await loadMercureInfo(() => apiClientMock)()(dispatch, getState); + + expect(apiClientMock.mercureInfo).not.toHaveBeenCalled(); + expect(dispatch).toHaveBeenCalledTimes(2); + expect(dispatch).toHaveBeenNthCalledWith(1, { type: GET_MERCURE_INFO_START }); + expect(dispatch).toHaveBeenNthCalledWith(2, { type: GET_MERCURE_INFO_ERROR }); + }); + it('calls API on success', async () => { const apiClientMock = createApiClientMock(Promise.resolve(mercureInfo)); + const getState = createGetStateMock(true); - await loadMercureInfo(() => apiClientMock)()(dispatch, getState()); + await loadMercureInfo(() => apiClientMock)()(dispatch, getState); expect(apiClientMock.mercureInfo).toHaveBeenCalledTimes(1); expect(dispatch).toHaveBeenCalledTimes(2); @@ -58,8 +75,9 @@ describe('mercureInfoReducer', () => { it('throws error on failure', async () => { const error = 'Error'; const apiClientMock = createApiClientMock(Promise.reject(error)); + const getState = createGetStateMock(true); - await loadMercureInfo(() => apiClientMock)()(dispatch, getState()); + await loadMercureInfo(() => apiClientMock)()(dispatch, getState); expect(apiClientMock.mercureInfo).toHaveBeenCalledTimes(1); expect(dispatch).toHaveBeenCalledTimes(2); diff --git a/test/short-urls/ShortUrlsList.test.js b/test/short-urls/ShortUrlsList.test.js index b66e9ac0..050238b9 100644 --- a/test/short-urls/ShortUrlsList.test.js +++ b/test/short-urls/ShortUrlsList.test.js @@ -9,7 +9,6 @@ describe('', () => { const ShortUrlsRow = () => ''; const listShortUrlsMock = jest.fn(); const resetShortUrlParamsMock = jest.fn(); - const realTimeUpdates = { enabled: true }; const ShortUrlsList = shortUrlsListCreator(ShortUrlsRow); @@ -38,7 +37,6 @@ describe('', () => { ] } mercureInfo={{ loading: true }} - settings={{ realTimeUpdates }} /> ); }); diff --git a/test/tags/TagsList.test.js b/test/tags/TagsList.test.js index b4ebcafa..c3bd3393 100644 --- a/test/tags/TagsList.test.js +++ b/test/tags/TagsList.test.js @@ -15,7 +15,7 @@ describe('', () => { const TagsList = createTagsList(TagCard); wrapper = shallow( - + ); return wrapper; diff --git a/test/visits/ShortUrlVisits.test.js b/test/visits/ShortUrlVisits.test.js index 615db013..38297826 100644 --- a/test/visits/ShortUrlVisits.test.js +++ b/test/visits/ShortUrlVisits.test.js @@ -14,7 +14,6 @@ describe('', () => { const history = { goBack: jest.fn(), }; - const realTimeUpdates = { enabled: true }; const VisitsStats = jest.fn(); beforeEach(() => { @@ -31,7 +30,6 @@ describe('', () => { shortUrlDetail={{}} cancelGetShortUrlVisits={identity} matchMedia={() => ({ matches: false })} - settings={{ realTimeUpdates }} /> ); }); diff --git a/test/visits/TagVisits.test.js b/test/visits/TagVisits.test.js index 8a871dae..96d3368f 100644 --- a/test/visits/TagVisits.test.js +++ b/test/visits/TagVisits.test.js @@ -13,7 +13,6 @@ describe('', () => { const history = { goBack: jest.fn(), }; - const realTimeUpdates = { enabled: true }; const VisitsStats = jest.fn(); beforeEach(() => { @@ -26,7 +25,6 @@ describe('', () => { history={history} tagVisits={{ loading: true, visits: [] }} cancelGetTagVisits={identity} - settings={{ realTimeUpdates }} /> ); }); From 52c56f7918ba2d21c641c571e18d87a3a684ab25 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Jun 2020 09:29:43 +0200 Subject: [PATCH 2/3] Created custom react hook that binds to mercure topic --- src/mercure/helpers/index.js | 5 +++++ src/short-urls/ShortUrlsList.js | 7 ++----- src/tags/TagsList.js | 7 ++----- src/visits/ShortUrlVisits.js | 7 ++----- src/visits/TagVisits.js | 9 +++------ 5 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/mercure/helpers/index.js b/src/mercure/helpers/index.js index 0cdd367c..ac824cc5 100644 --- a/src/mercure/helpers/index.js +++ b/src/mercure/helpers/index.js @@ -1,3 +1,4 @@ +import { useEffect } from 'react'; import { EventSourcePolyfill as EventSource } from 'event-source-polyfill'; export const bindToMercureTopic = (mercureInfo, topic, onMessage, onTokenExpired) => () => { @@ -21,3 +22,7 @@ export const bindToMercureTopic = (mercureInfo, topic, onMessage, onTokenExpired return () => es.close(); }; + +export const useMercureTopicBinding = (mercureInfo, topic, onMessage, onTokenExpired) => { + useEffect(bindToMercureTopic(mercureInfo, topic, onMessage, onTokenExpired), [ mercureInfo ]); +}; diff --git a/src/short-urls/ShortUrlsList.js b/src/short-urls/ShortUrlsList.js index 9df2250b..a7b275e9 100644 --- a/src/short-urls/ShortUrlsList.js +++ b/src/short-urls/ShortUrlsList.js @@ -8,7 +8,7 @@ import { serverType } from '../servers/prop-types'; import SortingDropdown from '../utils/SortingDropdown'; import { determineOrderDir } from '../utils/utils'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { bindToMercureTopic } from '../mercure/helpers'; +import { useMercureTopicBinding } from '../mercure/helpers'; import { shortUrlType } from './reducers/shortUrlsList'; import { shortUrlsListParamsType } from './reducers/shortUrlsListParams'; import './ShortUrlsList.scss'; @@ -116,10 +116,7 @@ const ShortUrlsList = (ShortUrlsRow) => { return resetShortUrlParams; }, []); - useEffect( - bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), - [ mercureInfo ] - ); + useMercureTopicBinding(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo); return ( diff --git a/src/tags/TagsList.js b/src/tags/TagsList.js index 4fc72741..9e6e9492 100644 --- a/src/tags/TagsList.js +++ b/src/tags/TagsList.js @@ -5,7 +5,7 @@ import Message from '../utils/Message'; import SearchField from '../utils/SearchField'; import { serverType } from '../servers/prop-types'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { bindToMercureTopic } from '../mercure/helpers'; +import { useMercureTopicBinding } from '../mercure/helpers'; import { TagsListType } from './reducers/tagsList'; const { ceil } = Math; @@ -30,10 +30,7 @@ const TagsList = (TagCard) => { useEffect(() => { forceListTags(); }, []); - useEffect( - bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), - [ mercureInfo ] - ); + useMercureTopicBinding(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo); const renderContent = () => { if (tagsList.loading) { diff --git a/src/visits/ShortUrlVisits.js b/src/visits/ShortUrlVisits.js index 131586fd..20e7aeea 100644 --- a/src/visits/ShortUrlVisits.js +++ b/src/visits/ShortUrlVisits.js @@ -2,7 +2,7 @@ import React, { useEffect } from 'react'; import PropTypes from 'prop-types'; import qs from 'qs'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { bindToMercureTopic } from '../mercure/helpers'; +import { useMercureTopicBinding } from '../mercure/helpers'; import { shortUrlVisitsType } from './reducers/shortUrlVisits'; import ShortUrlVisitsHeader from './ShortUrlVisitsHeader'; import { shortUrlDetailType } from './reducers/shortUrlDetail'; @@ -51,10 +51,7 @@ const ShortUrlVisits = (VisitsStats) => { useEffect(() => { getShortUrlDetail(shortCode, domain); }, []); - useEffect( - bindToMercureTopic(mercureInfo, `https://shlink.io/new-visit/${shortCode}`, createNewVisit, loadMercureInfo), - [ mercureInfo ], - ); + useMercureTopicBinding(mercureInfo, `https://shlink.io/new-visit/${shortCode}`, createNewVisit, loadMercureInfo); return ( diff --git a/src/visits/TagVisits.js b/src/visits/TagVisits.js index c570edf0..299e6e0c 100644 --- a/src/visits/TagVisits.js +++ b/src/visits/TagVisits.js @@ -1,7 +1,7 @@ -import React, { useEffect } from 'react'; +import React from 'react'; import PropTypes from 'prop-types'; import { MercureInfoType } from '../mercure/reducers/mercureInfo'; -import { bindToMercureTopic } from '../mercure/helpers'; +import { useMercureTopicBinding } from '../mercure/helpers'; import { TagVisitsType } from './reducers/tagVisits'; import TagVisitsHeader from './TagVisitsHeader'; @@ -35,10 +35,7 @@ const TagVisits = (VisitsStats, colorGenerator) => { const { tag } = params; const loadVisits = (dates) => getTagVisits(tag, dates); - useEffect( - bindToMercureTopic(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo), - [ mercureInfo ], - ); + useMercureTopicBinding(mercureInfo, 'https://shlink.io/new-visit', createNewVisit, loadMercureInfo); return ( From 54733eaa18203c2404318f89d7dfdc5a200af625 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Jun 2020 09:30:39 +0200 Subject: [PATCH 3/3] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b88f10d6..ee5beec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), #### Changed -* *Nothing* +* [#254](https://github.com/shlinkio/shlink-web-client/issues/254) Reduced duplication on code to handle mercure topics binding. #### Deprecated