From 79183b34a467524a1f224324cf8e1671f6a6079e Mon Sep 17 00:00:00 2001 From: Rory Abraham <47436092+roryabraham@users.noreply.github.com> Date: Fri, 12 Apr 2024 12:46:35 -0700 Subject: [PATCH] Merge pull request #40180 from Expensify/Rory-RevertRequestDeduping Remove all request de-duping logic (cherry picked from commit 9853832b0a2898b3399fb90ff7f6b14e3b20e5c9) --- src/libs/API/index.ts | 10 +------- src/libs/actions/App.ts | 4 +-- src/libs/actions/PersistedRequests.ts | 35 ++------------------------- src/libs/actions/Report.ts | 32 ++---------------------- src/types/onyx/Request.ts | 33 ++----------------------- tests/unit/PersistedRequests.ts | 19 --------------- 6 files changed, 8 insertions(+), 125 deletions(-) diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index 7d8cb348afbe..dbbcf790edf0 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -7,7 +7,6 @@ import * as Pusher from '@libs/Pusher/pusher'; import * as Request from '@libs/Request'; import CONST from '@src/CONST'; import type OnyxRequest from '@src/types/onyx/Request'; -import type {RequestConflictResolver} from '@src/types/onyx/Request'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import type {ApiRequest, ApiRequestCommandParameters, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types'; @@ -52,14 +51,8 @@ type OnyxData = { * @param [onyxData.successData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200. * @param [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200. * @param [onyxData.finallyData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200 or jsonCode !== 200. - * @param [conflictResolver] - callbacks used in special cases to detect and handle conflicting requests in the sequential queue */ -function write( - command: TCommand, - apiCommandParameters: ApiRequestCommandParameters[TCommand], - onyxData: OnyxData = {}, - conflictResolver: RequestConflictResolver = {}, -) { +function write(command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; @@ -90,7 +83,6 @@ function write( canCancel: true, }, ...onyxDataWithoutOptimisticData, - ...conflictResolver, }; // Write commands can be saved and retried, so push it to the SequentialQueue diff --git a/src/libs/actions/App.ts b/src/libs/actions/App.ts index 11bbc5c12f53..0ce3b4ce249b 100644 --- a/src/libs/actions/App.ts +++ b/src/libs/actions/App.ts @@ -238,9 +238,7 @@ function reconnectApp(updateIDFrom: OnyxEntry = 0) { params.updateIDFrom = updateIDFrom; } - API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect(), { - getConflictingRequests: (persistedRequests) => persistedRequests.filter((request) => request?.command === WRITE_COMMANDS.RECONNECT_APP), - }); + API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect()); }); } diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 8adb59824693..8894af8c374f 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -18,39 +18,8 @@ function clear() { } function save(requestToPersist: Request) { - const requests = [...persistedRequests, requestToPersist]; - - // identify and handle any existing requests that conflict with the new one - const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = requestToPersist; - if (getConflictingRequests) { - // Get all the requests, potentially including the one we're adding, which will always be at the end of the array - const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); - - // Identify conflicting requests according to logic bound to the new request - const conflictingRequests = getConflictingRequests(potentiallyConflictingRequests); - - conflictingRequests.forEach((conflictingRequest) => { - // delete the conflicting request - const index = requests.findIndex((req) => req === conflictingRequest); - if (index !== -1) { - requests.splice(index, 1); - } - - // Allow the new request to perform any additional cleanup for a cancelled request - handleConflictingRequest?.(conflictingRequest); - }); - } - - // Don't try to serialize conflict resolution functions - persistedRequests = requests.map((request) => { - delete request.getConflictingRequests; - delete request.handleConflictingRequest; - delete request.shouldIncludeCurrentRequest; - return request; - }); - - // Save the updated set of requests - Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); + persistedRequests.push(requestToPersist); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); } function remove(requestToRemove: Request) { diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index d2f85362baf8..64776352e337 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -753,19 +753,7 @@ function openReport( }); } else { // eslint-disable-next-line rulesdir/no-multiple-api-calls - API.write( - WRITE_COMMANDS.OPEN_REPORT, - parameters, - {optimisticData, successData, failureData}, - { - getConflictingRequests: (persistedRequests) => - // requests conflict only if: - // 1. they are OpenReport commands - // 2. they have the same reportID - // 3. they are not creating a report - all calls to OpenReport that create a report will be unique and have a unique createdReportActionID - persistedRequests.filter((request) => request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.reportID === reportID && !request.data?.createdReportActionID), - }, - ); + API.write(WRITE_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData}); } } @@ -1228,23 +1216,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { CachedPDFPaths.clearByKey(reportActionID); - API.write( - WRITE_COMMANDS.DELETE_COMMENT, - parameters, - {optimisticData, successData, failureData}, - { - getConflictingRequests: (persistedRequests) => { - const conflictingCommands = ( - isDeletedParentAction - ? [WRITE_COMMANDS.UPDATE_COMMENT] - : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT] - ) as string[]; - return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID); - }, - handleConflictingRequest: () => Onyx.update(successData), - shouldIncludeCurrentRequest: !isDeletedParentAction, - }, - ); + API.write(WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData}); } /** diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 74904a0f7194..f6e82d75db70 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -19,40 +19,11 @@ type RequestData = { successData?: OnyxUpdate[]; failureData?: OnyxUpdate[]; finallyData?: OnyxUpdate[]; - getConflictingRequests?: (persistedRequests: Request[]) => Request[]; - handleConflictingRequest?: (persistedRequest: Request) => unknown; resolve?: (value: Response) => void; reject?: (value?: unknown) => void; }; -type RequestConflictResolver = { - /** - * A callback that's provided with all the currently serialized functions in the sequential queue. - * Should return a subset of the requests passed in that conflict with the new request. - * Any conflicting requests will be cancelled and removed from the queue. - * - * @example - In ReconnectApp, you'd only want to have one instance of that command serialized to run on reconnect. The callback for that might look like this: - * (persistedRequests) => persistedRequests.filter((request) => request.command === 'ReconnectApp') - * */ - getConflictingRequests?: (persistedRequests: Request[]) => Request[]; - - /** - * Should the requests provided to getConflictingRequests include the new request? - * This is useful if the new request and an existing request cancel eachother out completely. - * - * @example - In DeleteComment, if you're deleting an optimistic comment, you'd want to cancel the optimistic AddComment call AND the DeleteComment call. - * */ - shouldIncludeCurrentRequest?: boolean; - - /** - * Callback to handle a single conflicting request. - * This is useful if you need to clean up some optimistic data for a request that was queue but never sent. - * In these cases the optimisticData will be applied immediately, but the successData, failureData, and/or finallyData will never be applied unless you do it manually in this callback. - */ - handleConflictingRequest?: (persistedRequest: Request) => unknown; -}; - -type Request = RequestData & OnyxData & RequestConflictResolver; +type Request = RequestData & OnyxData; export default Request; -export type {OnyxData, RequestType, RequestConflictResolver}; +export type {OnyxData, RequestType}; diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index f7b740d5e875..670625f65f97 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -22,25 +22,6 @@ describe('PersistedRequests', () => { expect(PersistedRequests.getAll().length).toBe(2); }); - it('save a new request with conflict resolution', () => { - const handleConflictingRequest = jest.fn(); - const newRequest = { - command: 'ThingA', - getConflictingRequests: (requests: Request[]) => requests, - handleConflictingRequest, - }; - const secondRequest = { - command: 'ThingB', - getConflictingRequests: (requests: Request[]) => requests, - shouldIncludeCurrentRequest: true, - }; - PersistedRequests.save(newRequest); - PersistedRequests.save(secondRequest); - expect(PersistedRequests.getAll().length).toBe(1); - expect(handleConflictingRequest).toHaveBeenCalledWith(request); - expect(handleConflictingRequest).toHaveBeenCalledTimes(1); - }); - it('remove a request from the PersistedRequests array', () => { PersistedRequests.remove(request); expect(PersistedRequests.getAll().length).toBe(0);