Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all request de-duping logic #40180

Merged
merged 6 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/libs/API/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<TCommand extends WriteCommand>(
command: TCommand,
apiCommandParameters: ApiRequestCommandParameters[TCommand],
onyxData: OnyxData = {},
conflictResolver: RequestConflictResolver = {},
) {
function write<TCommand extends WriteCommand>(command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}) {
Log.info('Called API write', false, {command, ...apiCommandParameters});
const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData;

Expand Down Expand Up @@ -90,7 +83,6 @@ function write<TCommand extends WriteCommand>(
canCancel: true,
},
...onyxDataWithoutOptimisticData,
...conflictResolver,
};

// Write commands can be saved and retried, so push it to the SequentialQueue
Expand Down
4 changes: 1 addition & 3 deletions src/libs/actions/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,7 @@ function reconnectApp(updateIDFrom: OnyxEntry<number> = 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());
});
}

Expand Down
35 changes: 2 additions & 33 deletions src/libs/actions/PersistedRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
}

function remove(requestToRemove: Request) {
Expand Down
32 changes: 2 additions & 30 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -763,19 +763,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});
}
}

Expand Down Expand Up @@ -1238,23 +1226,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});
}

/**
Expand Down
33 changes: 2 additions & 31 deletions src/types/onyx/Request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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};
19 changes: 0 additions & 19 deletions tests/unit/PersistedRequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading