chore: Prefetch consolidated api using service worker by dvj1988 · Pull Request #33306 · appsmithorg/appsmith

dvj1988 · 2024-05-09T05:25:08Z

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?

Yes
No

dvj1988 · 2024-05-09T05:25:26Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-09T05:25:48Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9012824234.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

dvj1988 · 2024-05-10T05:22:02Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-10T05:22:24Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9027698556.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

dvj1988 · 2024-05-10T05:30:33Z

app/client/craco.dev.config.js

  experiments: {
    cacheUnaffected: true,
  },
+  webpack: {


Revert this. This change is required only for testing

Does this change need to be reverted ?

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.)

@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.

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.

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

@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.

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

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.

@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

github-actions · 2024-05-10T05:36:11Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

…d-api

dvj1988 · 2024-05-10T09:55:35Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-10T09:55:58Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9030519052.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

github-actions · 2024-05-10T10:09:49Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

dvj1988 · 2024-05-12T18:10:08Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-12T18:10:28Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9053293027.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

github-actions · 2024-05-12T18:23:41Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

app/client/src/App.css

app/client/src/ce/AppRouter.tsx

dvj1988 · 2024-05-13T03:51:19Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-13T03:51:40Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9056956178.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

github-actions · 2024-05-13T04:04:48Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

github-actions · 2024-05-15T19:08:22Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

coderabbitai

Actionable comments posted: 2

app/client/src/utils/serviceWorkerUtils.js

coderabbitai

Actionable comments posted: 2

app/client/src/utils/serviceWorkerUtils.js

coderabbitai · 2024-05-15T19:31:57Z

app/client/src/utils/serviceWorkerUtils.js

+  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();
+    }
+  }


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.

Suggested change

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;

}

app/client/src/utils/serviceWorkerUtils.js

rajatagrawal · 2024-05-16T02:56:25Z

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.

gitdiffpatch.zip

app/client/src/utils/serviceWorkerUtils.js

coderabbitai

Actionable comments posted: 2

app/client/src/serviceWorker.js

dvj1988 · 2024-05-16T08:16:00Z

/build-deploy-preview skip-tests=true

github-actions · 2024-05-16T08:16:24Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9108873127.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 33306.
recreate: .

github-actions · 2024-05-16T08:30:58Z

Deploy-Preview-URL: https://ce-33306.dp.appsmith.com

KelvinOm · 2024-05-16T09:05:43Z

app/client/craco.dev.config.js

  experiments: {
    cacheUnaffected: true,
  },
+  webpack: {


@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.

This reverts commit 992790d.

WIP

e0cacf1

Diljit VJ added 3 commits May 9, 2024 17:51

WIP 2

c748784

WIP 3

ae3afec

Add view url

38eba74

dvj1988 changed the title ~~WIP~~ chore: Prefetch consolidated api using service worker May 10, 2024

github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label May 10, 2024

Move service worker to dev config

74964a8

dvj1988 commented May 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin' into chore/prefetch-consolidate…

f985f17

…d-api

Add url origin to the prefetching logic

2cfc7e7

Diljit VJ added 3 commits May 13, 2024 09:09

Revert changes in craco common

0ab583f

Revert a change in serviceWorker

dd9f8e6

Make changes to invalidate current bundle

5d27434

dvj1988 commented May 13, 2024

View reviewed changes

app/client/src/App.css Outdated Show resolved Hide resolved

app/client/src/ce/AppRouter.tsx Outdated Show resolved Hide resolved

Interim change

90580be

dvj1988 added the ok-to-test Required label for CI label May 13, 2024

coderabbitai bot reviewed May 15, 2024

View reviewed changes

app/client/src/utils/serviceWorkerUtils.js Show resolved Hide resolved

Update test cases

016c983

dvj1988 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels May 15, 2024

Diljit VJ added 2 commits May 16, 2024 00:53

Remove logs and add comments

4024a6f

Rename describe block

d06f4d9

coderabbitai bot reviewed May 15, 2024

View reviewed changes

rajatagrawal reviewed May 16, 2024

View reviewed changes

app/client/src/utils/serviceWorkerUtils.js Show resolved Hide resolved

rajatagrawal reviewed May 16, 2024

View reviewed changes

app/client/src/utils/serviceWorkerUtils.js Show resolved Hide resolved

PR comments

17b6d45

coderabbitai bot reviewed May 16, 2024

View reviewed changes

app/client/src/serviceWorker.js Show resolved Hide resolved

app/client/src/serviceWorker.js Show resolved Hide resolved

Add catch block to main request handler

30e2db9

dvj1988 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels May 16, 2024

KelvinOm requested changes May 16, 2024

View reviewed changes

dvj1988 requested a review from rajatagrawal May 16, 2024 09:14

rajatagrawal approved these changes May 16, 2024

View reviewed changes

dvj1988 requested a review from KelvinOm May 16, 2024 12:22

KelvinOm approved these changes May 16, 2024

View reviewed changes

dvj1988 merged commit 992790d into release May 16, 2024

dvj1988 deleted the chore/prefetch-consolidated-api branch May 16, 2024 12:33

dvj1988 pushed a commit that referenced this pull request May 21, 2024

Revert "chore: Prefetch consolidated api using service worker (#33306)"

f15d5d1

This reverts commit 992790d.

coderabbitai bot mentioned this pull request Oct 23, 2024

chore: fix errors related to service worker in dev mode #37012

Merged

2 tasks

This was referenced Jan 28, 2025

chore: make branchName a query param in consolidated api #38851

Merged

chore: add etag in consolidated api controller #38873

Merged

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

Conversation

dvj1988 commented May 9, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Automation

🔍 Cypress test results

Communication

Uh oh!

dvj1988 commented May 9, 2024

Uh oh!

github-actions bot commented May 9, 2024

Uh oh!

dvj1988 commented May 10, 2024

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KelvinOm May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvj1988 May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

dvj1988 commented May 10, 2024

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

github-actions bot commented May 10, 2024

Uh oh!

dvj1988 commented May 12, 2024

Uh oh!

github-actions bot commented May 12, 2024

Uh oh!

github-actions bot commented May 12, 2024

Uh oh!

Uh oh!

Uh oh!

dvj1988 commented May 13, 2024

Uh oh!

github-actions bot commented May 13, 2024

Uh oh!

github-actions bot commented May 13, 2024

Uh oh!

github-actions bot commented May 15, 2024

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot May 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajatagrawal commented May 16, 2024

Uh oh!

Uh oh!

coderabbitai bot left a comment

dvj1988 commented May 9, 2024 •

edited by github-actions bot

Loading

KelvinOm May 16, 2024 •

edited

Loading

dvj1988 May 16, 2024 •

edited

Loading