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

chore: Prefetch consolidated api using service worker #33306

Merged
merged 36 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e0cacf1
WIP
dvj1988 May 9, 2024
c748784
WIP 2
dvj1988 May 9, 2024
ae3afec
WIP 3
dvj1988 May 10, 2024
38eba74
Add view url
dvj1988 May 10, 2024
74964a8
Move service worker to dev config
dvj1988 May 10, 2024
f985f17
Merge remote-tracking branch 'origin' into chore/prefetch-consolidate…
dvj1988 May 10, 2024
2cfc7e7
Add url origin to the prefetching logic
dvj1988 May 12, 2024
0ab583f
Revert changes in craco common
dvj1988 May 13, 2024
dd9f8e6
Revert a change in serviceWorker
dvj1988 May 13, 2024
5d27434
Make changes to invalidate current bundle
dvj1988 May 13, 2024
90580be
Interim change
dvj1988 May 13, 2024
411c77a
Reset the cache on first fetch
dvj1988 May 13, 2024
c32a137
Merge remote-tracking branch 'origin' into chore/prefetch-consolidate…
dvj1988 May 13, 2024
4d9c7b2
Refactor code to handle git applications also
dvj1988 May 13, 2024
3bf22d7
Move methods to a util fn
dvj1988 May 13, 2024
094de9d
Add test cases
dvj1988 May 13, 2024
ff376d6
Revert unnecessary changes
dvj1988 May 13, 2024
b9f1ab5
PR comments
dvj1988 May 14, 2024
02967d5
Refactor and update test cases
dvj1988 May 14, 2024
cf64171
Remove caching of files indev mode of service worker
dvj1988 May 14, 2024
2e6f9b5
Add version number to cache name
dvj1988 May 14, 2024
7284f81
Implement skip cache
dvj1988 May 14, 2024
9300be7
Final checks and test cases
dvj1988 May 14, 2024
72e079f
Fix a typo
dvj1988 May 14, 2024
feae9fa
Add comments
dvj1988 May 14, 2024
7c6ab9d
Add cache expiry
dvj1988 May 15, 2024
ae9aa5b
PR comments
dvj1988 May 15, 2024
cd75730
PR comments
dvj1988 May 15, 2024
7cb6ea6
Add comments and change variable name
dvj1988 May 15, 2024
a32c11f
Implement the logic with mutex
dvj1988 May 15, 2024
ceaf254
refactor
dvj1988 May 15, 2024
016c983
Update test cases
dvj1988 May 15, 2024
4024a6f
Remove logs and add comments
dvj1988 May 15, 2024
d06f4d9
Rename describe block
dvj1988 May 15, 2024
17b6d45
PR comments
dvj1988 May 16, 2024
30e2db9
Add catch block to main request handler
dvj1988 May 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion app/client/craco.dev.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { merge } = require("webpack-merge");

const WorkboxPlugin = require("workbox-webpack-plugin");
const common = require("./craco.common.config.js");

module.exports = merge(common, {
Expand All @@ -21,4 +21,30 @@ module.exports = merge(common, {
experiments: {
cacheUnaffected: true,
},
webpack: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert this. This change is required only for testing

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change need to be reverted ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can leave it as is and enable it for dev mode.

Just FYI there are some warnings that popup in the terminal as result of enabling WorkboxPlugin in dev mode. This is a known issue for webpack watch mode (InjectManifest has been called multiple times, perhaps due to running webpack in --watch mode.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dvj1988 Does it affect the build time somehow? Did you check it?

We have some issues with building time, if it is not necessary, then I would prefer not to add anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not seen any issues with the dev server build time or hot reloads. Preferably we want to keep this so that we have the same behaviour of the service worker in the dev and prod modes.
Earlier we were using service worker to cache the static assets but this PR is prefetching and caching one of the apis. If there are any main bundle changes that would affect the behaviour of the service worker or vice versa we want to start catching them in the dev mode itself.
After the merge I will keep a track of whether this is affecting any devs local setup and can take a measure accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KelvinOm Do you have any specific scenario in mind to test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dvj1988 I am most interested in the start time (with and without cache) and the recompilation time. We also have problems OOM issues, this should not make the situation worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KelvinOm By cache do you mean lint cache?
How do you propose I measure these?

Copy link
Collaborator

@KelvinOm KelvinOm May 16, 2024

Choose a reason for hiding this comment

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

By cache I mean babel-loader cache it stored at app/client/node_modules/.cache/babel-loader. It is created when the webpack is running.

How do you propose I measure these?

I do not know a good way to do this. I'm measuring the time in terminal. We can use time like time yarn start. I couldn't run the webpack speed measure plugin because it doesn't work with craco. So we have to measure it manually.

Copy link
Contributor Author

@dvj1988 dvj1988 May 16, 2024

Choose a reason for hiding this comment

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

@KelvinOm I used the package called gnomon. And the time taken looks the same.

Start time without cache was replicated by following the steps

  • Checkout the branch
  • delete node_modules
  • install deps
  • comment out the cache settings in craco.dev.config
  • start the server
yarn start | gnomon --type=elapsed-total

Tests done in: Apple M1 Pro, RAM 32 GB

Branch Start time without cache Start time with cache Recompilation Time
release 190-192s 12-13s 4-6s
chore/prefetch-consolidated-api 190-192s 12-13s 4-6s

Start time without cache - release
Screenshot 2024-05-16 at 4 44 52 PM
Start time with cache - release
Screenshot 2024-05-16 at 4 47 10 PM
Start time without cache - chore/prefetch-consolidated-api
Screenshot 2024-05-16 at 5 39 47 PM
Start time with cache - chore/prefetch-consolidated-api
Screenshot 2024-05-16 at 5 12 39 PM

plugins: [
new WorkboxPlugin.InjectManifest({
swSrc: "./src/serviceWorker.js",
mode: "development",
swDest: "./pageService.js",
exclude: [
// Don’t cache source maps and PWA manifests.
// (These are the default values of the `exclude` option: https://developer.chrome.com/docs/workbox/reference/workbox-build/#type-WebpackPartial,
// so we need to specify them explicitly if we’re extending this array.)
/\.map$/,
/^manifest.*\.js$/,
// Don’t cache the root html file
/index\.html/,
// Don’t cache LICENSE.txt files emitted by CRA
// when a chunk includes some license comments
/LICENSE\.txt/,
// Don’t cache static icons as there are hundreds of them, and caching them all
// one by one (as the service worker does it) keeps the network busy for a long time
// and delays the service worker installation
/\/*\.svg$/,
/\.(js|css|html|png|jpg|jpeg|gif)$/, // Exclude JS, CSS, HTML, and image files
],
}),
],
},
});
1 change: 1 addition & 0 deletions app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
"appsmith-icons": "workspace:^",
"assert-never": "^1.2.1",
"astring": "^1.7.5",
"async-mutex": "^0.5.0",
"axios": "^1.6.0",
"classnames": "^2.3.1",
"clsx": "^1.2.1",
Expand Down
60 changes: 56 additions & 4 deletions app/client/src/serviceWorker.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import {
NetworkOnly,
StaleWhileRevalidate,
} from "workbox-strategies";
import {
getPrefetchConsolidatedApiRequest,
ConsolidatedApiCacheStrategy,
} from "utils/serviceWorkerUtils";

setCacheNameDetails({
prefix: "appsmith",
suffix: undefined,
suffix: "",
precache: "precache-v1",
runtime: "runtime",
googleAnalytics: "appsmith-ga",
Expand All @@ -34,6 +38,31 @@ self.__WB_DISABLE_DEV_DEBUG_LOGS = false;
skipWaiting();
clientsClaim();

const consolidatedApiCacheStrategy = new ConsolidatedApiCacheStrategy();

/**
*
* @param {ExtendableEvent} event
* @param {Request} request
* @param {URL} url
* @returns
*/
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(() => {
// Silently fail
});
}

const networkHandler = new NetworkOnly();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be part of if/else above this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. This is part of the normal fetch behaviour for all pages and the if block is only meant for edit and view page html requests.

return networkHandler.handle({ event, request });
};

// This route's caching seems too aggressive.
// TODO(abhinav): Figure out if this is really necessary.
// Maybe add the assets locally?
Expand All @@ -52,7 +81,30 @@ registerRoute(({ url }) => {
}, new StaleWhileRevalidate());

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

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

// If the response is cached, return the response
if (cachedResponse) {
return cachedResponse;
}

// If the response is not cached, fetch the response
const networkHandler = new NetworkOnly();
return networkHandler.handle({ event, request });
},
dvj1988 marked this conversation as resolved.
Show resolved Hide resolved
"GET",
);
177 changes: 177 additions & 0 deletions app/client/src/utils/serviceWorkerUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
/* eslint-disable no-console */
import { match } from "path-to-regexp";
import { Mutex } from "async-mutex";

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`;

/**
* Function to get the search query from the URL
* @param {string} search
* @param {string} key
* @returns {string | null}
*/
export const getSearchQuery = (search = "", key) => {
const params = new URLSearchParams(search);
return decodeURIComponent(params.get(key) || "");
};
dvj1988 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Function to match the path with the builder path
* @param {string} pathName
* @param {Object} options
* @param {boolean} options.end
* @returns {Match<object> | boolean}
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets see if we can share the util fns of this file with both service worker and and our main thread dataLayer for easier maintenance. This is not an action item now if this PR is a POC , it can be taken later as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure noted.

export const matchBuilderPath = (pathName, options) =>
match(BUILDER_PATH, options)(pathName) ||
match(BUILDER_PATH_DEPRECATED, options)(pathName) ||
match(BUILDER_CUSTOM_PATH, options)(pathName);

/**
* Function to match the path with the viewer path
* @param {string} pathName
* @returns {Match<object> | boolean}
*/
export const matchViewerPath = (pathName) =>
match(VIEWER_PATH)(pathName) ||
match(VIEWER_PATH_DEPRECATED)(pathName) ||
match(VIEWER_CUSTOM_PATH)(pathName);

/**
* Function to get the consolidated API search params
* @param {Match<object>} params
* @returns
*/
export const getConsolidatedAPISearchParams = (params = {}) => {
if (!params || !params?.pageId) {
return "";
}

const { applicationId, pageId } = params;
const searchParams = new URLSearchParams();

searchParams.append("defaultPageId", pageId);

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

return searchParams.toString();
};

/**
* Function to get the prefetch request for consolidated api
* @param {URL} url
* @returns {Request | null}
*/
export const getPrefetchConsolidatedApiRequest = (url) => {
if (!url) {
return null;
}

// Match the URL with the builder and viewer paths
const matchedBuilder = matchBuilderPath(url.pathname, { end: false });
const matchedViewer = matchViewerPath(url.pathname, { end: false });

// Get the branch name from the search query
const branchName = getSearchQuery(url.search, "branch");

let headers = new Headers();

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

// If the URL matches the builder path
if (matchedBuilder && matchedBuilder.params?.pageId) {
const searchParams = getConsolidatedAPISearchParams(matchedBuilder.params);
const requestUrl = `${url.origin}/api/v1/consolidated-api/edit?${searchParams}`;
const request = new Request(requestUrl, { method: "GET", headers });
return request;
}

// If the URL matches the viewer path
if (matchedViewer && matchedViewer.params?.pageId) {
const searchParams = getConsolidatedAPISearchParams(matchedViewer.params);
const requestUrl = `${url.origin}/api/v1/consolidated-api/view?${searchParams}`;
const request = new Request(requestUrl, { method: "GET", headers });
return request;
}

// Return null if the URL does not match the builder or viewer path
return null;
};

/**
* Cache strategy for Appsmith API
*/
export class ConsolidatedApiCacheStrategy {
cacheName = "prefetch-cache-v1";
cacheMaxAge = 2 * 60 * 1000; // 2 minutes in milliseconds

constructor() {
// Mutex to lock the fetch and cache operation
this.consolidatedApiFetchmutex = new Mutex();
}

/**
* Function to fetch and cache the consolidated API
* @param {Request} request
* @returns
*/
async cacheConsolidatedApi(request) {
// Acquire the lock
await this.consolidatedApiFetchmutex.acquire();
dvj1988 marked this conversation as resolved.
Show resolved Hide resolved
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.consolidatedApiFetchmutex.release();
}
}

async getCachedResponse(request) {
// Wait for the lock to be released
await this.consolidatedApiFetchmutex.waitForUnlock();
dvj1988 marked this conversation as resolved.
Show resolved Hide resolved
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 = new Date(dateHeader).getTime();
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;
}
}