feat(service-worker): support multiple apps in one domain #27080

sheikalthaf · 2018-11-13T19:55:53Z

Current behavior of the service-worker is caching data of multiple apps in cache DB file in a domain.
Due to this behavior service-worker makes the apps to break when switching from one app to other app in same domain. This PR will solve this issue by suffixing the base-href with ngsw string to separate caches files of an apps in a domain

Fixes issue #21388

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #21388
Currently service-worker doen't seperate the cache files based on base-href. Due to this caches files get clashes with multiple apps hosted in one domain

What is the new behavior?

In this PR seperating the caches files by suffing the base-href with ngsw string like this ngsw:<base-href> to seperate the caches files of an app. This will avoid the storing of cache data in same file for multiple apps in one domain

Does this PR introduce a breaking change?

Yes
No

Other information

sheikalthaf · 2018-11-13T19:57:04Z

@gkalpak Please review this PR and let me know any changes required. Also need help in test writing

AndrewKushnir · 2018-11-13T21:23:06Z

packages/service-worker/worker/src/adapter.ts

+   * in same domain with multiple apps
+   */
+  setBaseHref(baseHref: string) {
+    if (baseHref && ['/'].indexOf(baseHref) == -1) {


It looks like you can just compare base href with '/' string:

if (baseHref && baseHref === '/') {

@AndrewKushnir Hi,

ok now directly comparing with the '/'. Motivation is not to allow when base-href is not set that is '/'.

if (baseHref && baseHref !== '/') {

so that it work like current behavior

gkalpak · 2018-11-14T18:23:05Z

@sheikalthaf, I see you are still pushing changes to the PR. Is this ready for review?

sheikalthaf · 2018-11-14T18:24:08Z

@gkalpak yes it is now ready for review

gkalpak

The general direction this is going seems reasonable. Keep in mind that we also need tests that verify the new behavior.

packages/service-worker/worker/src/adapter.ts

gkalpak · 2018-11-14T18:27:30Z

packages/service-worker/worker/src/adapter.ts

+   * Suffixing the baseHref with `ngsw` string to avoid clash of cache files
+   * in same domain with multiple apps
+   */
+  setBaseHref(baseHref: string) {


IMO, this prefix should not be able to change at runtime. It should be determined during Adapter instantiation (as it remains constant throughout the lifetime of a SW instance).

gkalpak · 2018-11-14T18:30:37Z

packages/service-worker/worker/src/adapter.ts

+   * in same domain with multiple apps
+   */
+  setBaseHref(baseHref: string) {
+    if (baseHref && baseHref !== '/') {


AFAICT, baseHref should never be empty (it would be at least /).
But even so, why this check? Why not use /? Is it to retain backwards compatibility with existing caches (thus avoiding re-fetching all assets from the server)?

ok will remove this condition

gkalpak · 2018-11-14T18:31:17Z

packages/service-worker/worker/src/adapter.ts

+   */
+  setBaseHref(baseHref: string) {
+    if (baseHref && baseHref !== '/') {
+      const str = baseHref.replace(/^\//, '').replace(/\/$/, '').replace(/\//, ':');


Why not use it as-is?

This is why because '/' is not allowed in file name right?
if the base is something like this /app/de/ then we have to replace them with ':' . so the result will be app:de.

The cache name is not necessarily a file name. I didn't look at the spec, but Chrome seems to handle /s in cache names fine.

ok thanks for the clarification. will remove the replace condition and use it as-is

gkalpak · 2018-11-14T18:41:38Z

packages/service-worker/worker/src/db-cache.ts

@@ -23,16 +23,17 @@ export class CacheDatabase implements Database {
    if (this.tables.has(name)) {
      this.tables.delete(name);
    }
-    return this.scope.caches.delete(`ngsw:db:${name}`);
+    return this.scope.caches.delete(`${this.adapter.ngsw}:db:${name}`);


In most cases (with the sole exception of the control DB), CacheDatabase methods (such as delete, list, open) are called with names that already contain ngsw:<base-href>:, so prepending it again will get a little too verbose. But it is safer. I'm on the fence on this one.

Maybe we could strip the leading ngsw:<base-href>: from the name (so that baseHref is only included in the final cache name once).

gkalpak · 2018-11-14T18:42:57Z

packages/service-worker/worker/src/driver.ts

+     * Extract baseHref from scope
+     * send the baseHref to adapter to suffix with ngsw string
+     */
+    const baseHref = new URL(scope.registration.scope).pathname;


IMO, this should be moved to Adapter's instantiation.

Moved to adapter

gkalpak · 2018-11-14T18:46:33Z

packages/service-worker/worker/src/driver.ts

-    const oldSwCacheNames =
-        cacheNames.filter(name => /^ngsw:(?:active|staged|manifest:.+)$/.test(name));
+    const regex = new RegExp(`^${this.adapter.ngsw}:(?:active|staged|manifest:.+)$`);
+    const oldSwCacheNames = cacheNames.filter(name => regex.test(name));


This change should be reverted. The cleanupOldSwCaches() method matches caches created by an old SW implementation.
In fact, it might be a good idea to also match (and delete) caches that don't have the pathname after ngsw:. AFAICT, URL#pathname will always start with a /, so I think it is safe to delete all caches matching /^ngsw:(?!\/)/ (i.e. starting with ngsw: but not followed by /) - assuming we keep baseHref's leading / in cache names.

now changed the regex to above mentioned matching pattern. also adding baseHref without any changes to cache

sheikalthaf · 2018-11-17T15:44:45Z

@gkalpak i have updated the code and spec but i don't know how to test this because the hostUrl is hardcode. Any guidance is helpfull.
Also i want any guide to stepup the angular development in windows machine

gkalpak

Thx for the changes, @sheikalthaf. I left some more comments.
As discussed "offline", the development workflow on Windows is a little problematic atm. You should be able to run the tests with sh test.sh browserNoRouter inside git bash (which comes bundled with git-for-windows).

gkalpak · 2018-11-20T12:44:11Z

packages/service-worker/worker/src/adapter.ts

@@ -54,6 +61,15 @@ export class Adapter {
  timeout(ms: number): Promise<void> {
    return new Promise<void>(resolve => { setTimeout(() => resolve(), ms); });
  }
+
+  /**
+   * Suffixing the baseHref with `ngsw` string to avoid clash of cache files


Actually, it is the other way around: Suffixing ngsw (aka the cache name prefix) with the baseHref.

gkalpak · 2018-11-20T12:44:24Z

packages/service-worker/worker/src/adapter.ts

+   * Suffixing the baseHref with `ngsw` string to avoid clash of cache files
+   * in same domain with multiple apps
+   */
+  suffixBaseHref(hostUrl: string) {


This should not be a method, since the value better remain the same throughout the lifetime of the adapter. cacheNamePrefix should be set once inside the constructor and be a readonly property.

changes commited

gkalpak · 2018-11-20T13:10:34Z

packages/service-worker/worker/testing/scope.ts

@@ -254,6 +256,11 @@ export class SwTestHarness implements ServiceWorkerGlobalScope, Adapter, Context
    return promise;
  }

+  suffixBaseHref(hostUrl: string) {


Again, no need for a method here. We can interact with the property directly.

@gkalpak removed this method

googlebot · 2018-11-21T13:19:37Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

gkalpak

I've gone ahead and made some changes:

Rebased on latest master.
Squashed your commits into one.
Added some more commits (including tests).

gkalpak · 2018-11-21T11:20:35Z

packages/service-worker/worker/src/adapter.ts

@@ -13,6 +13,12 @@
 * from the global scope.
 */
 export class Adapter {
+  cacheNamePrefix: string;


Can you also mark this property as readonly (to better convey that it is not supposed or expected to change at runtime).

Already it is marked as readonly in my branch.

gkalpak · 2018-11-21T11:23:07Z

packages/service-worker/worker/src/adapter.ts

@@ -13,6 +13,12 @@
 * from the global scope.
 */
 export class Adapter {
+  cacheNamePrefix: string;
+  constructor(scope: ServiceWorkerGlobalScope) {
+    // Suffixing the baseHref with `ngsw` string to avoid clash of cache files


I would change to:

Suffixing `ngsw` with the baseHref to avoid clash of cache names for SWs with different scopes on the same domain.

Meaningfull message 😊

gkalpak · 2018-11-21T11:26:39Z

packages/service-worker/worker/testing/scope.ts

@@ -77,6 +77,7 @@ export class SwTestHarness implements ServiceWorkerGlobalScope, Adapter, Context
  readonly clients = new MockClients();
  private eventHandlers = new Map<string, Function>();
  private skippedWaiting = true;
+  cacheNamePrefix: string;


Mark as readonly to better mimic the actual implementation.
(Note: We might still overwrite it during tests, but at least one has to be very intentional about it 😁)

…)` optional

…ame on browser and Node.js

… that do not support SW The tests will not be run anyway, so the artifacts are never used and there might be errors if creating the testing artifacts relies on APIs that are not available in that environment (e.g. `URL`).

…a domain Previously, it was not possible to have multiple apps (using `@angular/service-worker`) on different subpaths of the same domain, because each SW would overwrite the caches of the others (even though their scope was different). This commit fixes it by ensuring that the cache names created by the SW are different for each scope. Fixes angular#21388

…ths of a domain

…paths of a domain

This commit also ensures that the correct implementation is used on environments that do not support `URL` (e.g. Node.js).

gkalpak · 2019-03-20T21:32:26Z

Rebased. Thx again for working on this, @sheikalthaf 👍

merge-assistance: Needs CLA override.

sheikalthaf · 2019-03-21T05:17:44Z

@IgorMinar @gkalpak Thanks for your awesome support in helping contributors. On this PR special thanks to @gkalpak making this possible with his great help.

#loveAngular ❤

googlebot · 2019-03-21T15:29:52Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

…ame on browser and Node.js (#27080) PR Close #27080

… that do not support SW (#27080) The tests will not be run anyway, so the artifacts are never used and there might be errors if creating the testing artifacts relies on APIs that are not available in that environment (e.g. `URL`). PR Close #27080

…a domain (#27080) Previously, it was not possible to have multiple apps (using `@angular/service-worker`) on different subpaths of the same domain, because each SW would overwrite the caches of the others (even though their scope was different). This commit fixes it by ensuring that the cache names created by the SW are different for each scope. Fixes #21388 PR Close #27080

…ts (#27080) PR Close #27080

…paths of a domain (#27080) PR Close #27080

#27080) This commit also ensures that the correct implementation is used on environments that do not support `URL` (e.g. Node.js). PR Close #27080

…)` optional (angular#27080) PR Close angular#27080

…ame on browser and Node.js (angular#27080) PR Close angular#27080

… that do not support SW (angular#27080) The tests will not be run anyway, so the artifacts are never used and there might be errors if creating the testing artifacts relies on APIs that are not available in that environment (e.g. `URL`). PR Close angular#27080

…a domain (angular#27080) Previously, it was not possible to have multiple apps (using `@angular/service-worker`) on different subpaths of the same domain, because each SW would overwrite the caches of the others (even though their scope was different). This commit fixes it by ensuring that the cache names created by the SW are different for each scope. Fixes angular#21388 PR Close angular#27080

…ts (angular#27080) PR Close angular#27080

…paths of a domain (angular#27080) PR Close angular#27080

angular#27080) This commit also ensures that the correct implementation is used on environments that do not support `URL` (e.g. Node.js). PR Close angular#27080

angular-automatic-lock-bot · 2019-09-14T20:53:08Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Nov 13, 2018

AndrewKushnir reviewed Nov 13, 2018

View reviewed changes

gkalpak added feature state: WIP target: major area: service-worker labels Nov 14, 2018

ngbot bot added this to the needsTriage milestone Nov 14, 2018

gkalpak requested changes Nov 14, 2018

View reviewed changes

sheikalthaf force-pushed the sw-multiple-app branch from f60f85e to 21d39bb Compare November 17, 2018 09:49

gkalpak requested changes Nov 20, 2018

View reviewed changes

gkalpak force-pushed the sw-multiple-app branch from fef0acb to 926178a Compare November 21, 2018 13:19

googlebot added cla: no and removed cla: yes labels Nov 21, 2018

gkalpak force-pushed the sw-multiple-app branch 4 times, most recently from 2931507 to 534632c Compare November 22, 2018 17:22

gkalpak reviewed Nov 22, 2018

View reviewed changes

gkalpak force-pushed the sw-multiple-app branch from 534632c to 3cc8cf7 Compare November 22, 2018 17:32

gkalpak approved these changes Nov 22, 2018

View reviewed changes

gkalpak requested review from alxhub and IgorMinar November 22, 2018 18:36

gkalpak assigned IgorMinar Nov 22, 2018

gkalpak added action: review and removed state: WIP labels Nov 23, 2018

gkalpak and others added 8 commits March 20, 2019 23:29

refactor(service-worker): make second parameter to `Adapter#parseUrl(…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

8a04e87

…)` optional

test(service-worker): ensure SwTestHarness#parseUrl() behaves the s…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

a5faf1e

…ame on browser and Node.js

fixup! feat(service-worker): support multiple apps on different subpa…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

917616d

…ths of a domain

test(service-worker): make it easy to use a different SW scope in tests

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

7d1e107

test(service-worker): test support for multiple apps on different sub…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

d5ffce7

…paths of a domain

gkalpak force-pushed the sw-multiple-app branch from 010e61d to 7be6483 Compare March 20, 2019 21:29

IgorMinar added cla: yes and removed cla: no labels Mar 21, 2019

matsko closed this in b3dda0e Mar 21, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

feat(service-worker): support multiple apps in one domain #27080

feat(service-worker): support multiple apps in one domain #27080

Conversation

sheikalthaf commented Nov 13, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

sheikalthaf commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Nov 14, 2018

sheikalthaf commented Nov 14, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheikalthaf commented Nov 17, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheikalthaf Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

googlebot commented Nov 21, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Mar 20, 2019

sheikalthaf commented Mar 21, 2019 • edited Loading

googlebot commented Mar 21, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

sheikalthaf Nov 20, 2018 •

edited

Loading

sheikalthaf commented Mar 21, 2019 •

edited

Loading