From 80aac85bfe410134c17098b240729e5f7e570f95 Mon Sep 17 00:00:00 2001 From: Jalil Arfaoui Date: Tue, 31 Aug 2021 01:01:36 +0200 Subject: [PATCH 1/2] perf(matching): filter matching contexts lists at once --- src/app/background/store/sagas/tab.ts | 32 ++--- src/app/background/store/selectors/index.ts | 6 - src/libs/domain/matchingContext.ts | 40 +++--- ...indMatchingOffersAccordingToPreferences.ts | 130 +++++++----------- 4 files changed, 88 insertions(+), 120 deletions(-) diff --git a/src/app/background/store/sagas/tab.ts b/src/app/background/store/sagas/tab.ts index e050cb22d..f8b370948 100644 --- a/src/app/background/store/sagas/tab.ts +++ b/src/app/background/store/sagas/tab.ts @@ -26,10 +26,7 @@ import { TabAction } from 'libs/store/actions'; import { createErrorAction } from 'libs/store/actions/helpers'; -import { - filterContextsMatchingTabContent, - MatchingContext -} from 'libs/domain/matchingContext'; +import { MatchingContext, filterContexts } from 'libs/domain/matchingContext'; import { NoticeWithContributor, StatefulNoticeWithContributor, @@ -46,9 +43,11 @@ import { getIgnoredNotices, getNoticesToDisplay } from 'app/background/store/selectors/prefs'; -import { getContextsMatchingUrl } from 'app/background/store/selectors'; import { getInstallationDetails } from 'app/background/store/selectors/installationDetails'; -import { isTabAuthorized } from 'app/background/store/selectors/resources'; +import { + getMatchingContexts, + isTabAuthorized +} from 'app/background/store/selectors/resources'; import { getNbSubscriptions } from 'app/background/store/selectors/subscriptions.selectors'; import serviceMessageSaga from './serviceMessage.saga'; import sendToTabSaga from './lib/sendToTab.saga'; @@ -83,20 +82,19 @@ export function* tabSaga({ meta: { tab } }: TabAction) { export function* matchContextSaga({ meta: { tab } }: MatchContextAction) { try { yield put(clearServiceMessage(tab)); - const contextsMatchingUrl = yield select(state => - getContextsMatchingUrl(state)(tab.url) - ); - if (contextsMatchingUrl.length >= 1) { - const contextsMatchingContent = yield call( - filterContextsMatchingTabContent, - tab, - contextsMatchingUrl - ); + const matchingContexts = yield select(getMatchingContexts); + + const contextsMatchingUrlAndContent: MatchingContext[] = yield call( + filterContexts, + matchingContexts, + tab + ); - yield put(contextTriggered(contextsMatchingContent, tab)); + if (contextsMatchingUrlAndContent.length >= 1) { + yield put(contextTriggered(contextsMatchingUrlAndContent, tab)); } else { - yield put(contextNotTriggered(contextsMatchingUrl, tab)); + yield put(contextNotTriggered(contextsMatchingUrlAndContent, tab)); } } catch (e) { yield put(matchContextFailure(e, tab)); diff --git a/src/app/background/store/selectors/index.ts b/src/app/background/store/selectors/index.ts index b0d992186..462e7b558 100644 --- a/src/app/background/store/selectors/index.ts +++ b/src/app/background/store/selectors/index.ts @@ -1,18 +1,12 @@ import { PersistedState } from 'redux-persist'; import { createSelector } from 'reselect'; import { InstallationDetails } from 'libs/domain/installation'; -import { filterContextsMatchingUrl } from 'libs/domain/matchingContext'; import { getNotice } from 'libs/domain/notice'; import { BackgroundState } from 'app/background/store/reducers'; -import { getMatchingContexts } from './resources'; import { getInstallationDetails } from './installationDetails'; import { areTosAccepted, getRead } from './prefs'; import { getNoticesIdsOnTab } from './tabs'; -export const getContextsMatchingUrl = (state: BackgroundState) => ( - url: string -) => filterContextsMatchingUrl(url, getMatchingContexts(state)); - export const isAnUpdate = createSelector( getInstallationDetails, ({ reason }: InstallationDetails) => reason !== 'install' diff --git a/src/libs/domain/matchingContext.ts b/src/libs/domain/matchingContext.ts index 0d92947ec..74efe357c 100644 --- a/src/libs/domain/matchingContext.ts +++ b/src/libs/domain/matchingContext.ts @@ -37,14 +37,6 @@ export const urlMatchesContext = ( } }; -export const filterContextsMatchingUrl = ( - url: string, - matchingContexts: MatchingContext[] -) => - matchingContexts.filter((context: MatchingContext) => - urlMatchesContext(url, context) - ); - export const doesTabContentMatchExpression = async ( tab: Tab, xpath: string, @@ -57,18 +49,28 @@ export const doesTabContentMatchExpression = async ( ]) : Promise.resolve(true); -export const filterContextsMatchingTabContent = async ( - tab: Tab, - matchingContexts: MatchingContext[] -) => { +export const filterContexts = async ( + matchingContexts: MatchingContext[], + tab: Tab +): Promise => { const responses = await Promise.all( - matchingContexts.map(({ xpath, id }: MatchingContext) => { - return ( - typeof xpath === 'undefined' || - doesTabContentMatchExpression(tab, xpath, id) - ); + matchingContexts.map((matchingContext: MatchingContext): + | Promise + | MatchingContext + | false => { + if (urlMatchesContext(tab.url, matchingContext)) { + if (!matchingContext.xpath) return matchingContext; + + return doesTabContentMatchExpression( + tab, + matchingContext.xpath, + matchingContext.id + ).then((matchesContent: boolean) => + matchesContent ? matchingContext : false + ); + } + return false; }) ); - - return matchingContexts.filter((_, index) => responses[index]); + return responses.filter(Boolean) as MatchingContext[]; }; diff --git a/test/app/lmem/findMatchingOffersAccordingToPreferences.ts b/test/app/lmem/findMatchingOffersAccordingToPreferences.ts index dcb44ad56..05f895862 100644 --- a/test/app/lmem/findMatchingOffersAccordingToPreferences.ts +++ b/test/app/lmem/findMatchingOffersAccordingToPreferences.ts @@ -1,7 +1,7 @@ import chai from 'chai'; import { - filterContextsMatchingUrl, - MatchingContext + MatchingContext, + urlMatchesContext } from 'libs/domain/matchingContext'; import generateMatchingContext from 'test/fakers/generateMatchingContext'; @@ -15,110 +15,84 @@ const offers: MatchingContext[] = [ const matchingURL = 'https://www.samsung.com/blabla'; const nonMatchingURL = 'https://soundcloud.com/capt-lovelace/meteo-marine'; -describe('filterContextsMatchingUrl', function() { +describe('urlMatchesContext', function() { it('should be case insensitive', () => { - const offersWithWeirdCase: MatchingContext[] = [ - generateMatchingContext({ urlRegex: 's.*' }), - generateMatchingContext({ urlRegex: 'SamSung' }), - generateMatchingContext({ urlRegex: 'doesNotMatch' }) - ]; - - const matches = filterContextsMatchingUrl(matchingURL, offersWithWeirdCase); - - expect(matches).to.be.an('array'); - expect(matches).to.be.of.length(2); - expect(matches[0]).to.equal(offersWithWeirdCase[0]); - expect(matches[1]).to.equal(offersWithWeirdCase[1]); + expect( + urlMatchesContext( + matchingURL, + generateMatchingContext({ urlRegex: 's.*' }) + ) + ).to.equal(true); + + expect( + urlMatchesContext( + matchingURL, + generateMatchingContext({ urlRegex: 'SamSung' }) + ) + ).to.equal(true); + + expect( + urlMatchesContext( + matchingURL, + generateMatchingContext({ urlRegex: 'doesNotMatch' }) + ) + ).to.equal(false); }); describe('exclusion', () => { it('should exclude matching exclusion of otherwise matching url', () => { - const offersWithExclusion: MatchingContext[] = [ - generateMatchingContext({ - urlRegex: 'samsung', - excludeUrlRegex: 'blabla' - }) - ]; - - const matches = filterContextsMatchingUrl( - matchingURL, - offersWithExclusion - ); + const offersWithExclusion: MatchingContext = generateMatchingContext({ + urlRegex: 'samsung', + excludeUrlRegex: 'blabla' + }); - expect(matches).to.be.an('array'); - expect(matches).to.be.of.length(0); + const matches = urlMatchesContext(matchingURL, offersWithExclusion); + + expect(matches).to.equal(false); }); it('should not exclude non matching exclusion of matching url', () => { - const offersWithExclusion = [ - generateMatchingContext({ - urlRegex: 'samsung', - excludeUrlRegex: 'nono' - }) - ]; - - const matches = filterContextsMatchingUrl( - matchingURL, - offersWithExclusion - ); + const offersWithExclusion = generateMatchingContext({ + urlRegex: 'samsung', + excludeUrlRegex: 'nono' + }); - expect(matches).to.be.an('array'); - expect(matches).to.be.of.length(1); - expect(matches[0]).to.equal(offersWithExclusion[0]); + const matches = urlMatchesContext(matchingURL, offersWithExclusion); + + expect(matches).to.equal(true); }); it('should exclude its matching context if regex is invalid', () => { - const offersWithExclusion: MatchingContext[] = [ - generateMatchingContext({ - urlRegex: 'samsung', - excludeUrlRegex: 'isNasty)' - }), // SyntaxError: Invalid RegExp: Unmatched ')', - ...offers - ]; - - const matches = filterContextsMatchingUrl( - matchingURL, - offersWithExclusion - ); + const offersWithExclusion: MatchingContext = generateMatchingContext({ + urlRegex: 'samsung', + excludeUrlRegex: 'isNasty)' + }); - expect(matches).to.be.an('array'); - expect(matches).to.be.of.length(1); - expect(matches[0]).to.equal(offersWithExclusion[1]); + const matches = urlMatchesContext(matchingURL, offersWithExclusion); + + expect(matches).to.equal(false); }); }); describe('invalid regex', () => { - const nastyOffers = [ - generateMatchingContext({ - urlRegex: 'isNasty' - }) - ].concat(offers); // SyntaxError: Invalid RegExp: Unmatched ')' + const nastyOffers = generateMatchingContext({ + urlRegex: 'isNasty)' + }); it('should not screw up the matching engine', () => { - const matches = filterContextsMatchingUrl(matchingURL, nastyOffers); - - expect(filterContextsMatchingUrl).to.not.throw(SyntaxError); - - expect(matches).to.be.an('array'); - expect(matches).to.be.of.length(1); - expect(matches[0]).to.equal(nastyOffers[1]); + expect(urlMatchesContext(matchingURL, nastyOffers)).to.equal(false); }); }); describe('empty prefs', () => { it('should match when the url matches an offer', () => { - const matching = filterContextsMatchingUrl(matchingURL, offers); - - expect(matching).to.be.an('array'); - expect(matching).to.be.of.length(1); - expect(matching[0]).to.equal(offers[0]); + expect(urlMatchesContext(matchingURL, offers[0])).to.equal(true); + expect(urlMatchesContext(matchingURL, offers[1])).to.equal(false); }); it('should not match when the url does not match any offer', () => { - const matching = filterContextsMatchingUrl(nonMatchingURL, offers); - - expect(matching).to.be.an('array'); - expect(matching).to.be.of.length(0); + expect(urlMatchesContext(nonMatchingURL, offers[0])).to.equal(false); + expect(urlMatchesContext(nonMatchingURL, offers[1])).to.equal(false); }); }); }); From c0da152db857b313fce4c02d0e0dd9b65d0e54e9 Mon Sep 17 00:00:00 2001 From: Jalil Arfaoui Date: Tue, 21 Sep 2021 16:39:35 +0200 Subject: [PATCH 2/2] refactor: simplify return type of `filterContexts` inner map --- src/libs/domain/matchingContext.ts | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/libs/domain/matchingContext.ts b/src/libs/domain/matchingContext.ts index 74efe357c..f5387ad43 100644 --- a/src/libs/domain/matchingContext.ts +++ b/src/libs/domain/matchingContext.ts @@ -54,23 +54,22 @@ export const filterContexts = async ( tab: Tab ): Promise => { const responses = await Promise.all( - matchingContexts.map((matchingContext: MatchingContext): - | Promise - | MatchingContext - | false => { - if (urlMatchesContext(tab.url, matchingContext)) { - if (!matchingContext.xpath) return matchingContext; + matchingContexts.map( + (matchingContext: MatchingContext): Promise => { + if (urlMatchesContext(tab.url, matchingContext)) { + if (!matchingContext.xpath) return Promise.resolve(matchingContext); - return doesTabContentMatchExpression( - tab, - matchingContext.xpath, - matchingContext.id - ).then((matchesContent: boolean) => - matchesContent ? matchingContext : false - ); + return doesTabContentMatchExpression( + tab, + matchingContext.xpath, + matchingContext.id + ).then((matchesContent: boolean) => + matchesContent ? matchingContext : false + ); + } + return Promise.resolve(false); } - return false; - }) + ) ); return responses.filter(Boolean) as MatchingContext[]; };