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

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

Closed
wants to merge 8 commits into from

Conversation

@sheikalthaf
Copy link
Contributor

commented Nov 13, 2018

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:

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

@googlebot googlebot added the cla: yes label Nov 13, 2018

@sheikalthaf

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

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

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

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Nov 13, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 14, 2018

Author Contributor

@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

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

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

@sheikalthaf

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@gkalpak yes it is now ready for review

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

@gkalpak
Copy link
Member

left a comment

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

Show resolved Hide resolved packages/service-worker/worker/src/adapter.ts Outdated
* Suffixing the baseHref with `ngsw` string to avoid clash of cache files
* in same domain with multiple apps
*/
setBaseHref(baseHref: string) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

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

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 15, 2018

Author Contributor

ok will remove this condition

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

Why not use it as-is?

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 15, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 15, 2018

Member

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.

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 15, 2018

Author Contributor

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

@@ -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}`);

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

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

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 17, 2018

Author Contributor

Moved to adapter

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 comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 14, 2018

Member

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.

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 17, 2018

Author Contributor

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

@sheikalthaf sheikalthaf force-pushed the sheikalthaf:sw-multiple-app branch from f60f85e to 21d39bb Nov 17, 2018

@sheikalthaf

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

@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
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 20, 2018

Member

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

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 20, 2018

Member

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.

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 20, 2018

Author Contributor

changes commited

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

suffixBaseHref(hostUrl: string) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 20, 2018

Member

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 20, 2018

Author Contributor

@gkalpak removed this method

@gkalpak gkalpak force-pushed the sheikalthaf:sw-multiple-app branch from fef0acb to 926178a Nov 21, 2018

@googlebot

This comment has been minimized.

Copy link

commented Nov 21, 2018

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.

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

@gkalpak gkalpak force-pushed the sheikalthaf:sw-multiple-app branch 4 times, most recently from 2931507 to 534632c Nov 21, 2018

@gkalpak
Copy link
Member

left a comment

I've gone ahead and made some changes:

  • Rebased on latest master.
  • Squashed your commits into one.
  • Added some more commits (including tests).
@@ -13,6 +13,12 @@
* from the global scope.
*/
export class Adapter {
cacheNamePrefix: string;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 22, 2018

Member

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 22, 2018

Author Contributor

Already it is marked as readonly in my branch.

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 22, 2018

Member

I would change to:

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

This comment has been minimized.

Copy link
@sheikalthaf

sheikalthaf Nov 22, 2018

Author Contributor

Meaningfull message 😊

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 22, 2018

Member

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

@gkalpak gkalpak force-pushed the sheikalthaf:sw-multiple-app branch from 534632c to 3cc8cf7 Nov 22, 2018

@sheikalthaf sheikalthaf requested a review from angular/fw-core as a code owner Mar 20, 2019

gkalpak and others added some commits Mar 20, 2019

test(service-worker): do not create testing artifacts on environments…
… 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`).
feat(service-worker): support multiple apps on different subpaths of …
…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 #21388
refactor(service-worker): use `Adapter#parseUrl()` for all URL parsing
This commit also ensures that the correct implementation is used on
environments that do not support `URL` (e.g. Node.js).

@gkalpak gkalpak force-pushed the sheikalthaf:sw-multiple-app branch from 010e61d to 7be6483 Mar 20, 2019

@gkalpak

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

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

merge-assistance: Needs CLA override.

@sheikalthaf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

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

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

@googlebot

This comment has been minimized.

Copy link

commented Mar 21, 2019

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.

@matsko matsko closed this in b3dda0e Mar 21, 2019

matsko added a commit that referenced this pull request Mar 21, 2019

matsko added a commit that referenced this pull request Mar 21, 2019

test(service-worker): do not create testing artifacts on environments…
… 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

matsko added a commit that referenced this pull request Mar 21, 2019

feat(service-worker): support multiple apps on different subpaths of …
…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

matsko added a commit that referenced this pull request Mar 21, 2019

matsko added a commit that referenced this pull request Mar 21, 2019

refactor(service-worker): use `Adapter#parseUrl()` for all URL parsing (
#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

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

test(service-worker): do not create testing artifacts on environments…
… 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

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

feat(service-worker): support multiple apps on different subpaths of …
…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

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

refactor(service-worker): use `Adapter#parseUrl()` for all URL parsing (
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.