-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from all commits
e0cacf1
c748784
ae3afec
38eba74
74964a8
f985f17
2cfc7e7
0ab583f
dd9f8e6
5d27434
90580be
411c77a
c32a137
4d9c7b2
3bf22d7
094de9d
ff376d6
b9f1ab5
02967d5
cf64171
2e6f9b5
7284f81
9300be7
72e079f
feae9fa
7c6ab9d
ae9aa5b
cd75730
7cb6ea6
a32c11f
ceaf254
016c983
4024a6f
d06f4d9
17b6d45
30e2db9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to be part of if/else above this line ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? | ||
|
@@ -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", | ||
); |
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} | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 atapp/client/node_modules/.cache/babel-loader
. It is created when the webpack is running.I do not know a good way to do this. I'm measuring the time in terminal. We can use
time
liketime yarn start
. I couldn't run the webpack speed measure plugin because it doesn't work withcraco
. So we have to measure it manually.There was a problem hiding this comment.
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
yarn start | gnomon --type=elapsed-total
Tests done in: Apple M1 Pro, RAM 32 GB
Start time without cache - release
Start time with cache - release
Start time without cache - chore/prefetch-consolidated-api
Start time with cache - chore/prefetch-consolidated-api