-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9012824234. |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9027698556. |
@@ -21,4 +21,32 @@ module.exports = merge(common, { | |||
experiments: { | |||
cacheUnaffected: true, | |||
}, | |||
webpack: { |
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 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.
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
- 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
Start time with cache - release
Start time without cache - chore/prefetch-consolidated-api
Start time with cache - chore/prefetch-consolidated-api
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9030519052. |
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9053293027. |
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9056956178. |
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9101177210. |
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.
Actionable comments posted: 1
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
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.
Actionable comments posted: 2
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.
Actionable comments posted: 2
async cacheConsolidatedApi(request) { | ||
// Acquire the lock | ||
await this.consolidatedApiFetchmutex.acquire(); | ||
|
||
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 this.cache.put(request, clonedResponse); | ||
} | ||
} catch (error) { | ||
// Delete the existing cache if the fetch fails | ||
await this.cache.delete(request); | ||
} finally { | ||
// Release the lock | ||
this.consolidatedApiFetchmutex.release(); | ||
} | ||
} |
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.
Add a return statement to indicate success or failure in cacheConsolidatedApi
.
To improve clarity, add a return statement that indicates whether the caching was successful.
async cacheConsolidatedApi(request) {
// Acquire the lock
await this.consolidatedApiFetchmutex.acquire();
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 this.cache.put(request, clonedResponse);
+ return true;
}
} catch (error) {
// Delete the existing cache if the fetch fails
await this.cache.delete(request);
+ return false;
} finally {
// Release the lock
this.consolidatedApiFetchmutex.release();
}
+ return false;
}
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.
async cacheConsolidatedApi(request) { | |
// Acquire the lock | |
await this.consolidatedApiFetchmutex.acquire(); | |
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 this.cache.put(request, clonedResponse); | |
} | |
} catch (error) { | |
// Delete the existing cache if the fetch fails | |
await this.cache.delete(request); | |
} finally { | |
// Release the lock | |
this.consolidatedApiFetchmutex.release(); | |
} | |
} | |
async cacheConsolidatedApi(request) { | |
// Acquire the lock | |
await this.consolidatedApiFetchmutex.acquire(); | |
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 this.cache.put(request, clonedResponse); | |
return true; | |
} | |
} catch (error) { | |
// Delete the existing cache if the fetch fails | |
await this.cache.delete(request); | |
return false; | |
} finally { | |
// Release the lock | |
this.consolidatedApiFetchmutex.release(); | |
} | |
return false; | |
} |
Suggested refactor to comply with single responsibility principle and some suggested naming changes to make code more generic. The code logic is fine and untouched. Just a suggestion on an iteration of code organization. I have suggested the changes in a patch file. The patch can be applied locally to see the suggested changes. |
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.
Actionable comments posted: 2
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9108873127. |
Deploy-Preview-URL: https://ce-33306.dp.appsmith.com |
@@ -21,4 +21,32 @@ module.exports = merge(common, { | |||
experiments: { | |||
cacheUnaffected: true, | |||
}, | |||
webpack: { |
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.
Description
Prefetch the consolidated API using the service worker. This api is prefetched for the view and edit urls.
Fixes Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9108829019
Commit: 30e2db9
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?