Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 1 addition & 1 deletion app/client/craco.build.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const plugins = [];

plugins.push(
new WorkboxPlugin.InjectManifest({
swSrc: "./src/serviceWorker.js",
swSrc: "./src/serviceWorker.ts",
mode: "development",
swDest: "./pageService.js",
maximumFileSizeToCacheInBytes: 11 * 1024 * 1024,
Expand Down
2 changes: 1 addition & 1 deletion app/client/craco.dev.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module.exports = merge(common, {
webpack: {
plugins: [
new WorkboxPlugin.InjectManifest({
swSrc: "./src/serviceWorker.js",
swSrc: "./src/serviceWorker.ts",
mode: "development",
swDest: "./pageService.js",
exclude: [
Expand Down
1 change: 1 addition & 0 deletions app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@
"@types/moment-timezone": "^0.5.10",
"@types/nanoid": "^2.0.0",
"@types/node": "^10.12.18",
"@types/node-fetch": "^2.6.11",
"@types/node-forge": "^0.10.0",
"@types/object-hash": "^2.2.1",
"@types/pg": "^8.10.2",
Expand Down
252 changes: 252 additions & 0 deletions app/client/src/ce/utils/serviceWorkerUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
import { Mutex } from "async-mutex";
import { APP_MODE } from "entities/App";
import type { Match, TokensToRegexpOptions } from "path-to-regexp";
import { match } from "path-to-regexp";

export const BUILDER_PATH = `/app/:applicationSlug/:pageSlug(.*\-):pageId/edit`;
export const BUILDER_CUSTOM_PATH = `/app/:customSlug(.*\-):pageId/edit`;
export const VIEWER_PATH = `/app/:applicationSlug/:pageSlug(.*\-):pageId`;
export const VIEWER_CUSTOM_PATH = `/app/:customSlug(.*\-):pageId`;
export const BUILDER_PATH_DEPRECATED = `/applications/:applicationId/pages/:pageId/edit`;
export const VIEWER_PATH_DEPRECATED = `/applications/:applicationId/pages/:pageId`;

interface TMatchResult {
pageId?: string;
applicationId?: string;
}

export interface TApplicationParams {
origin: string;
pageId?: string;
applicationId?: string;
branchName: string;
appMode: APP_MODE;
}

type TApplicationParamsOrNull = TApplicationParams | null;

export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a regular expression literal for better performance and readability.

- export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/");
+ export const cachedApiUrlRegex = /\/api\/v1\/consolidated-api\//;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/");
export const cachedApiUrlRegex = /\/api\/v1\/consolidated-api\//;
Tools
Biome

[error] 28-28: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)

Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.


/**
* Function to match the path with the builder path
*/
export const matchBuilderPath = (
pathName: string,
options: TokensToRegexpOptions,
) =>
match<TMatchResult>(BUILDER_PATH, options)(pathName) ||
match<TMatchResult>(BUILDER_PATH_DEPRECATED, options)(pathName) ||
match<TMatchResult>(BUILDER_CUSTOM_PATH, options)(pathName);

/**
* Function to match the path with the viewer path
*/
export const matchViewerPath = (pathName: string) =>
match<TMatchResult>(VIEWER_PATH)(pathName) ||
match<TMatchResult>(VIEWER_PATH_DEPRECATED)(pathName) ||
match<TMatchResult>(VIEWER_CUSTOM_PATH)(pathName);

/**
* returns the value in the query string for a key
*/
export const getSearchQuery = (search = "", key: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust the default parameter to follow the last required parameter.

- export const getSearchQuery = (search = "", key: string) => {
+ export const getSearchQuery = (key: string, search = "") => {
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const getSearchQuery = (search = "", key: string) => {
export const getSearchQuery = (key: string, search = "") => {
Tools
Biome

[error] 52-52: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)

The last required parameter is here:

A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.

const params = new URLSearchParams(search);
return decodeURIComponent(params.get(key) || "");
};

export const getApplicationParamsFromUrl = (
url: URL,
): TApplicationParamsOrNull => {
// Get the branch name from the query string
const branchName = getSearchQuery(url.search, "branch");

const matchedBuilder: Match<TMatchResult> = matchBuilderPath(url.pathname, {
end: false,
});
const matchedViewer: Match<TMatchResult> = matchViewerPath(url.pathname);

if (matchedBuilder) {
return {
origin: url.origin,
pageId: matchedBuilder.params.pageId,
applicationId: matchedBuilder.params.applicationId,
branchName,
appMode: APP_MODE.EDIT,
};
}

if (matchedViewer) {
return {
origin: url.origin,
pageId: matchedViewer.params.pageId,
applicationId: matchedViewer.params.applicationId,
branchName,
appMode: APP_MODE.PUBLISHED,
};
}

return null;
};

/**
* Function to get the prefetch request for consolidated api
*/
export const getConsolidatedApiPrefetchRequest = (
applicationProps: TApplicationParams,
) => {
const { applicationId, appMode, branchName, origin, pageId } =
applicationProps;

const headers = new Headers();
const searchParams = new URLSearchParams();

if (!pageId) {
return null;
}

searchParams.append("defaultPageId", pageId);

if (applicationId) {
searchParams.append("applicationId", applicationId);
}

// Add the branch name to the headers
if (branchName) {
headers.append("Branchname", branchName);
}

// If the URL matches the builder path
if (appMode === APP_MODE.EDIT) {
const requestUrl = `${origin}/api/v1/consolidated-api/edit?${searchParams.toString()}`;
const request = new Request(requestUrl, { method: "GET", headers });
return request;
}

// If the URL matches the viewer path
if (appMode === APP_MODE.PUBLISHED) {
const requestUrl = `${origin}/api/v1/consolidated-api/view?${searchParams.toString()}`;
const request = new Request(requestUrl, { method: "GET", headers });
return request;
}

return null;
};

/**
* Function to get the prefetch request for module apis
*/
export const getPrefetchModuleApiRequests = (
// eslint-disable-next-line @typescript-eslint/no-unused-vars
applicationProps: TApplicationParams,
): Request[] => {
return [];
};

/**
* Cache strategy for Appsmith API
*/
export class PrefetchApiCacheStrategy {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This can be renamed to PrefetchApiCachingService

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or PrefetchApiService

cacheName = "prefetch-cache-v1";
cacheMaxAge = 2 * 60 * 1000; // 2 minutes in milliseconds
// Mutex to lock the fetch and cache operation
prefetchFetchMutexMap = new Map<string, Mutex>();

constructor() {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the unnecessary constructor to simplify the class definition.

- constructor() {}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor() {}
Tools
Biome

[error] 161-161: This constructor is unnecessary. (lint/complexity/noUselessConstructor)

Unsafe fix: Remove the unnecessary constructor.


// Function to get the request key
getRequestKey = (request: Request) => {
const headersKey = Array.from(request.headers.entries())
.map(([key, value]) => `${key}:${value}`)
.join(",");
return `${request.method}:${request.url}:${headersKey}`;
};

// Function to acquire the fetch mutex for a request
aqcuireFetchMutex = async (request: Request) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could consider moving mutex functions into a separate file to improve readability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and have separate jest test cases for the mutex file

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mutex map is a state specific to this class. The methods aqcuireFetchMutex, waitForUnlock and releaseFetchMutex are member fns which are member fns which access or mutate this state. I would suggest keeping this as part of this class itself.

const requestKey = this.getRequestKey(request);
let mutex = this.prefetchFetchMutexMap.get(requestKey);

if (!mutex) {
mutex = new Mutex();
this.prefetchFetchMutexMap.set(requestKey, mutex);
}

return mutex.acquire();
};

// Function to wait for the lock to be released for a request
waitForUnlock = async (request: Request) => {
const requestKey = this.getRequestKey(request);
const mutex = this.prefetchFetchMutexMap.get(requestKey);

if (mutex) {
return mutex.waitForUnlock();
}
};

// Function to release the fetch mutex for a request
releaseFetchMutex = (request: Request) => {
const requestKey = this.getRequestKey(request);
const mutex = this.prefetchFetchMutexMap.get(requestKey);

if (mutex) {
mutex.release();
}
};

/**
* Function to fetch and cache the consolidated API
*/
async cacheApi(request: Request) {
// Acquire the lock
await this.aqcuireFetchMutex(request);
const prefetchApiCache = await caches.open(this.cacheName);
try {
const response = await fetch(request);

if (response.ok) {
// Clone the response as the response can be consumed only once
const clonedResponse = response.clone();
// Put the response in the cache
await prefetchApiCache.put(request, clonedResponse);
}
} catch (error) {
// Delete the existing cache if the fetch fails
await prefetchApiCache.delete(request);
} finally {
// Release the lock
this.releaseFetchMutex(request);
Comment thread
dvj1988 marked this conversation as resolved.
}
}

/**
* Function to get the cached response for the request
*/
async getCachedResponse(request: Request) {
// Wait for the lock to be released
await this.waitForUnlock(request);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitForUnlock returns a null value in some cases. Is it needed to handle that scenario ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are no prefetched requests for a certain request, then the waitForUnlock will return undefined. The fn waitForUnlock is async and can be awaited in these cases as well. Therefore no changes are required here.

const prefetchApiCache = await caches.open(this.cacheName);
// Check if the response is already in cache
const cachedResponse = await prefetchApiCache.match(request);

if (cachedResponse) {
const dateHeader = cachedResponse.headers.get("date");
const cachedTime = dateHeader ? new Date(dateHeader).getTime() : 0;
const currentTime = Date.now();

const isCacheValid = currentTime - cachedTime < this.cacheMaxAge;

if (isCacheValid) {
// Delete the cache as this is a one-time cache
await prefetchApiCache.delete(request);
// Return the cached response
return cachedResponse;
}

// If the cache is not valid, delete the cache
await prefetchApiCache.delete(request);
}

return null;
}
}
1 change: 1 addition & 0 deletions app/client/src/ee/utils/serviceWorkerUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "../../ce/utils/serviceWorkerUtils";
75 changes: 48 additions & 27 deletions app/client/src/serviceWorker.js → app/client/src/serviceWorker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@ import {
StaleWhileRevalidate,
} from "workbox-strategies";
import {
getPrefetchConsolidatedApiRequest,
ConsolidatedApiCacheStrategy,
} from "utils/serviceWorkerUtils";
cachedApiUrlRegex,
getApplicationParamsFromUrl,
getConsolidatedApiPrefetchRequest,
getPrefetchModuleApiRequests,
PrefetchApiCacheStrategy,
} from "@appsmith/utils/serviceWorkerUtils";
Comment thread
dvj1988 marked this conversation as resolved.
import type { RouteHandlerCallback } from "workbox-core/types";

setCacheNameDetails({
prefix: "appsmith",
Expand All @@ -30,33 +34,52 @@ const regexMap = {

/* eslint-disable no-restricted-globals */
// Note: if you need to filter out some files from precaching,

// do that in craco.build.config.js → workbox webpack plugin options
const toPrecache = self.__WB_MANIFEST;
const toPrecache = (self as any).__WB_MANIFEST;
precacheAndRoute(toPrecache);

self.__WB_DISABLE_DEV_DEBUG_LOGS = false;
self.__WB_DISABLE_DEV_LOGS = false;
skipWaiting();
clientsClaim();

const consolidatedApiCacheStrategy = new ConsolidatedApiCacheStrategy();
const prefetchApiCacheStrategy = new PrefetchApiCacheStrategy();

/**
*
* @param {ExtendableEvent} event
* @param {Request} request
* @param {URL} url
* @returns
* Route handler callback for HTML pages.
* This callback is responsible for prefetching the API requests for the application page.
*/
const handleFetchHtml = async (event, request, url) => {
// Get the prefetch consolidated api request if the url matches the builder or viewer path
const prefetchConsolidatedApiRequest = getPrefetchConsolidatedApiRequest(url);

if (prefetchConsolidatedApiRequest) {
consolidatedApiCacheStrategy
.cacheConsolidatedApi(prefetchConsolidatedApiRequest)
.catch(() => {
const htmlRouteHandlerCallback: RouteHandlerCallback = async ({
event,
request,
url,
}) => {
// Extract application params from the URL
const applicationParams = getApplicationParamsFromUrl(url);

// If application params are present, prefetch the API requests for the application
if (applicationParams) {
// Prefetch the consolidated API request
const consolidatedApiPrefetchRequest =
getConsolidatedApiPrefetchRequest(applicationParams);

if (consolidatedApiPrefetchRequest) {
prefetchApiCacheStrategy
.cacheApi(consolidatedApiPrefetchRequest)
.catch(() => {
// Silently fail
});
}

// Prefetch the module API requests
const moduleApiPrefetchRequests =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this logic be extracted into a generic functions that returns an array of all prefetch requests and fetches them ? Consolidated apis and prefetch apis.

getPrefetchModuleApiRequests(applicationParams);

moduleApiPrefetchRequests.forEach((prefetchRequest) => {
prefetchApiCacheStrategy.cacheApi(prefetchRequest).catch(() => {
// Silently fail
});
});
Comment thread
dvj1988 marked this conversation as resolved.
}

const networkHandler = new NetworkOnly();
Expand All @@ -81,21 +104,19 @@ registerRoute(({ url }) => {
}, new StaleWhileRevalidate());

registerRoute(
new Route(
({ request, sameOrigin }) => {
return sameOrigin && request.destination === "document";
},
async ({ event, request, url }) => handleFetchHtml(event, request, url),
),
new Route(({ request, sameOrigin }) => {
return sameOrigin && request.destination === "document";
}, htmlRouteHandlerCallback),
);

// Route for fetching the API directly
registerRoute(
new RegExp("/api/v1/consolidated-api/"),
// Intercept requests to the consolidated API and module instances API
cachedApiUrlRegex,
async ({ event, request }) => {
// Check for cached response
const cachedResponse =
await consolidatedApiCacheStrategy.getCachedResponse(request);
await prefetchApiCacheStrategy.getCachedResponse(request);

// If the response is cached, return the response
if (cachedResponse) {
Expand Down
Loading