refactor(docs-infra): make `SwUpdatesService` depend on `LocationService` (#39651)
Previously, the `LocationService` depended on the `SwUpdatesService`. This felt backwards, since `LocationService` is a more low-level and basic service and should not be depending on a service for a higher-level, specific feature (ServiceWorkers). This commit inverses the relation, making `SwUpdatesService` depend on `LocationService` instead. PR Close #39651
This commit is contained in:
George Kalpakas
Andrew Kushnir
parent
824f051843
commit
305d05545a
|
|
@ -1,10 +1,8 @@
|
|||
import { Injector } from '@angular/core';
|
||||
import { Location, LocationStrategy, PlatformLocation } from '@angular/common';
|
||||
import { MockLocationStrategy } from '@angular/common/testing';
|
||||
import { Subject } from 'rxjs';
|
||||
|
||||
import { GaService } from 'app/shared/ga.service';
|
||||
import { SwUpdatesService } from 'app/sw-updates/sw-updates.service';
|
||||
import { LocationService } from './location.service';
|
||||
import { ScrollService } from './scroll.service';
|
||||
|
||||
|
|
@ -12,25 +10,22 @@ describe('LocationService', () => {
|
|||
let injector: Injector;
|
||||
let location: MockLocationStrategy;
|
||||
let service: LocationService;
|
||||
let swUpdates: MockSwUpdatesService;
|
||||
let scrollService: MockScrollService;
|
||||
|
||||
beforeEach(() => {
|
||||
injector = Injector.create({
|
||||
providers: [
|
||||
{ provide: LocationService, deps: [GaService, Location, ScrollService, PlatformLocation, SwUpdatesService] },
|
||||
{ provide: LocationService, deps: [GaService, Location, ScrollService, PlatformLocation] },
|
||||
{ provide: Location, deps: [LocationStrategy, PlatformLocation] },
|
||||
{ provide: GaService, useClass: TestGaService, deps: [] },
|
||||
{ provide: LocationStrategy, useClass: MockLocationStrategy, deps: [] },
|
||||
{ provide: PlatformLocation, useClass: MockPlatformLocation, deps: [] },
|
||||
{ provide: SwUpdatesService, useClass: MockSwUpdatesService, deps: [] },
|
||||
{ provide: ScrollService, useClass: MockScrollService, deps: [] }
|
||||
]
|
||||
});
|
||||
|
||||
location = injector.get(LocationStrategy) as unknown as MockLocationStrategy;
|
||||
service = injector.get(LocationService);
|
||||
swUpdates = injector.get(SwUpdatesService) as unknown as MockSwUpdatesService;
|
||||
scrollService = injector.get(ScrollService);
|
||||
});
|
||||
|
||||
|
|
@ -244,7 +239,7 @@ describe('LocationService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('go', () => {
|
||||
describe('go()', () => {
|
||||
|
||||
it('should update the location', () => {
|
||||
service.go('some-new-url');
|
||||
|
|
@ -295,19 +290,19 @@ describe('LocationService', () => {
|
|||
expect(goExternalSpy).toHaveBeenCalledWith(externalUrl);
|
||||
});
|
||||
|
||||
it('should do a "full page navigation" and remove the stored scroll position when navigating to ' +
|
||||
'internal URLs only if a ServiceWorker update has been activated', () => {
|
||||
it('should do a "full page navigation" if requested and remove the stored scroll position ' +
|
||||
'when navigating to internal URLs only', () => {
|
||||
const goExternalSpy = spyOn(service, 'goExternal');
|
||||
const removeStoredScrollInfoSpy = spyOn(scrollService, 'removeStoredScrollInfo');
|
||||
|
||||
// Internal URL - No ServiceWorker update
|
||||
// Internal URL - No full page navigation requested
|
||||
service.go('some-internal-url');
|
||||
expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
|
||||
expect(goExternalSpy).not.toHaveBeenCalled();
|
||||
expect(location.path(true)).toEqual('some-internal-url');
|
||||
|
||||
// Internal URL - ServiceWorker update
|
||||
swUpdates.updateActivated.next('foo');
|
||||
// Internal URL - Full page navigation requested
|
||||
service.fullPageNavigationNeeded();
|
||||
service.go('other-internal-url');
|
||||
expect(goExternalSpy).toHaveBeenCalledWith('other-internal-url');
|
||||
expect(removeStoredScrollInfoSpy).toHaveBeenCalled();
|
||||
|
|
@ -319,13 +314,13 @@ describe('LocationService', () => {
|
|||
const externalUrl = 'http://some/far/away/land';
|
||||
const otherExternalUrl = 'http://some/far/far/away/land';
|
||||
|
||||
// External URL - No ServiceWorker update
|
||||
// External URL - No full page navigation requested
|
||||
service.go(externalUrl);
|
||||
expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
|
||||
expect(goExternalSpy).toHaveBeenCalledWith(externalUrl);
|
||||
|
||||
// External URL - ServiceWorker update
|
||||
swUpdates.updateActivated.next('foo');
|
||||
// External URL - Full page navigation requested
|
||||
service.fullPageNavigationNeeded();
|
||||
service.go(otherExternalUrl);
|
||||
expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
|
||||
expect(goExternalSpy).toHaveBeenCalledWith(otherExternalUrl);
|
||||
|
|
@ -341,7 +336,7 @@ describe('LocationService', () => {
|
|||
|
||||
});
|
||||
|
||||
describe('search', () => {
|
||||
describe('search()', () => {
|
||||
it('should read the query from the current location.path', () => {
|
||||
location.simulatePopState('a/b/c?foo=bar&moo=car');
|
||||
expect(service.search()).toEqual({ foo: 'bar', moo: 'car' });
|
||||
|
|
@ -378,7 +373,7 @@ describe('LocationService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('setSearch', () => {
|
||||
describe('setSearch()', () => {
|
||||
let platformLocation: MockPlatformLocation;
|
||||
|
||||
beforeEach(() => {
|
||||
|
|
@ -416,7 +411,7 @@ describe('LocationService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('handleAnchorClick', () => {
|
||||
describe('handleAnchorClick()', () => {
|
||||
let anchor: HTMLAnchorElement;
|
||||
|
||||
beforeEach(() => {
|
||||
|
|
@ -630,10 +625,6 @@ class MockPlatformLocation {
|
|||
replaceState = jasmine.createSpy('PlatformLocation.replaceState');
|
||||
}
|
||||
|
||||
class MockSwUpdatesService {
|
||||
updateActivated = new Subject<string>();
|
||||
}
|
||||
|
||||
class MockScrollService {
|
||||
removeStoredScrollInfo() { }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -5,7 +5,6 @@ import { ReplaySubject } from 'rxjs';
|
|||
import { map, tap } from 'rxjs/operators';
|
||||
|
||||
import { GaService } from 'app/shared/ga.service';
|
||||
import { SwUpdatesService } from 'app/sw-updates/sw-updates.service';
|
||||
import { ScrollService } from './scroll.service';
|
||||
|
||||
@Injectable()
|
||||
|
|
@ -13,7 +12,7 @@ export class LocationService {
|
|||
|
||||
private readonly urlParser = document.createElement('a');
|
||||
private urlSubject = new ReplaySubject<string>(1);
|
||||
private swUpdateActivated = false;
|
||||
private fullPageNavigation = false;
|
||||
|
||||
currentUrl = this.urlSubject
|
||||
.pipe(map(url => this.stripSlashes(url)));
|
||||
|
|
@ -27,16 +26,22 @@ export class LocationService {
|
|||
private gaService: GaService,
|
||||
private location: Location,
|
||||
private scrollService: ScrollService,
|
||||
private platformLocation: PlatformLocation,
|
||||
swUpdates: SwUpdatesService) {
|
||||
private platformLocation: PlatformLocation) {
|
||||
|
||||
this.urlSubject.next(location.path(true));
|
||||
|
||||
this.location.subscribe(state => {
|
||||
return this.urlSubject.next(state.url || '');
|
||||
});
|
||||
}
|
||||
|
||||
swUpdates.updateActivated.subscribe(() => this.swUpdateActivated = true);
|
||||
/**
|
||||
* Signify that a full page navigation is needed (instead of a regular in-app navigation).
|
||||
*
|
||||
* This will happen on the next user-initiated navigation.
|
||||
*/
|
||||
fullPageNavigationNeeded(): void {
|
||||
this.fullPageNavigation = true;
|
||||
}
|
||||
|
||||
// TODO: ignore if url-without-hash-or-search matches current location?
|
||||
|
|
@ -46,9 +51,9 @@ export class LocationService {
|
|||
if (/^http/.test(url)) {
|
||||
// Has http protocol so leave the site
|
||||
this.goExternal(url);
|
||||
} else if (this.swUpdateActivated) {
|
||||
// (Do a "full page navigation" if a ServiceWorker update has been activated)
|
||||
// We need to remove stored Position in order to be sure to scroll to the Top position
|
||||
} else if (this.fullPageNavigation) {
|
||||
// Do a "full page navigation".
|
||||
// We need to remove the stored scroll position to ensure we scroll to the top.
|
||||
this.scrollService.removeStoredScrollInfo();
|
||||
this.goExternal(url);
|
||||
} else {
|
||||
|
|
@ -118,7 +123,6 @@ export class LocationService {
|
|||
* `AppComponent`, whose element contains all the of the application and so captures all
|
||||
* link clicks both inside and outside the `DocViewerComponent`.
|
||||
*/
|
||||
|
||||
handleAnchorClick(anchor: HTMLAnchorElement, button = 0, ctrlKey = false, metaKey = false) {
|
||||
|
||||
// Check for modifier keys and non-left-button, which indicate the user wants to control navigation
|
||||
|
|
|
|||
|
|
@ -3,7 +3,9 @@ import { discardPeriodicTasks, fakeAsync, tick } from '@angular/core/testing';
|
|||
import { SwUpdate } from '@angular/service-worker';
|
||||
import { Subject } from 'rxjs';
|
||||
|
||||
import { LocationService } from 'app/shared/location.service';
|
||||
import { Logger } from 'app/shared/logger.service';
|
||||
import { MockLocationService } from 'testing/location.service';
|
||||
import { MockLogger } from 'testing/logger.service';
|
||||
import { SwUpdatesService } from './sw-updates.service';
|
||||
|
||||
|
|
@ -11,6 +13,7 @@ import { SwUpdatesService } from './sw-updates.service';
|
|||
describe('SwUpdatesService', () => {
|
||||
let injector: Injector;
|
||||
let appRef: MockApplicationRef;
|
||||
let location: MockLocationService;
|
||||
let service: SwUpdatesService;
|
||||
let swu: MockSwUpdate;
|
||||
let checkInterval: number;
|
||||
|
|
@ -24,12 +27,14 @@ describe('SwUpdatesService', () => {
|
|||
const setup = (isSwUpdateEnabled: boolean) => {
|
||||
injector = Injector.create({providers: [
|
||||
{ provide: ApplicationRef, useClass: MockApplicationRef, deps: [] },
|
||||
{ provide: LocationService, useFactory: () => new MockLocationService(''), deps: [] },
|
||||
{ provide: Logger, useClass: MockLogger, deps: [] },
|
||||
{ provide: SwUpdate, useFactory: () => new MockSwUpdate(isSwUpdateEnabled), deps: [] },
|
||||
{ provide: SwUpdatesService, deps: [ApplicationRef, Logger, SwUpdate] }
|
||||
{ provide: SwUpdatesService, deps: [ApplicationRef, LocationService, Logger, SwUpdate] }
|
||||
]});
|
||||
|
||||
appRef = injector.get(ApplicationRef) as unknown as MockApplicationRef;
|
||||
location = injector.get(LocationService) as unknown as MockLocationService;
|
||||
service = injector.get(SwUpdatesService);
|
||||
swu = injector.get(SwUpdate) as unknown as MockSwUpdate;
|
||||
checkInterval = (service as any).checkInterval;
|
||||
|
|
@ -100,16 +105,18 @@ describe('SwUpdatesService', () => {
|
|||
discardPeriodicTasks();
|
||||
})));
|
||||
|
||||
it('should emit on `updateActivated` when an update has been activated', run(() => {
|
||||
const activatedVersions: (string|undefined)[] = [];
|
||||
service.updateActivated.subscribe(v => activatedVersions.push(v));
|
||||
|
||||
it('should request a full page navigation when an update has been activated', run(() => {
|
||||
swu.$$availableSubj.next({available: {hash: 'foo'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'bar'}});
|
||||
swu.$$availableSubj.next({available: {hash: 'baz'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'qux'}});
|
||||
expect(location.fullPageNavigationNeeded).toHaveBeenCalledTimes(0);
|
||||
|
||||
expect(activatedVersions).toEqual(['bar', 'qux']);
|
||||
swu.$$activatedSubj.next({current: {hash: 'bar'}});
|
||||
expect(location.fullPageNavigationNeeded).toHaveBeenCalledTimes(1);
|
||||
|
||||
swu.$$availableSubj.next({available: {hash: 'baz'}});
|
||||
expect(location.fullPageNavigationNeeded).toHaveBeenCalledTimes(1);
|
||||
|
||||
swu.$$activatedSubj.next({current: {hash: 'qux'}});
|
||||
expect(location.fullPageNavigationNeeded).toHaveBeenCalledTimes(2);
|
||||
}));
|
||||
|
||||
describe('when `SwUpdate` is not enabled', () => {
|
||||
|
|
@ -135,16 +142,13 @@ describe('SwUpdatesService', () => {
|
|||
expect(swu.activateUpdate).not.toHaveBeenCalled();
|
||||
})));
|
||||
|
||||
it('should never emit on `updateActivated`', runDeactivated(() => {
|
||||
const activatedVersions: (string|undefined)[] = [];
|
||||
service.updateActivated.subscribe(v => activatedVersions.push(v));
|
||||
|
||||
it('should never request a full page navigation', runDeactivated(() => {
|
||||
swu.$$availableSubj.next({available: {hash: 'foo'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'bar'}});
|
||||
swu.$$availableSubj.next({available: {hash: 'baz'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'qux'}});
|
||||
|
||||
expect(activatedVersions).toEqual([]);
|
||||
expect(location.fullPageNavigationNeeded).not.toHaveBeenCalled();
|
||||
}));
|
||||
});
|
||||
|
||||
|
|
@ -185,17 +189,17 @@ describe('SwUpdatesService', () => {
|
|||
expect(swu.activateUpdate).not.toHaveBeenCalled();
|
||||
})));
|
||||
|
||||
it('should stop emitting on `updateActivated`', run(() => {
|
||||
const activatedVersions: (string|undefined)[] = [];
|
||||
service.updateActivated.subscribe(v => activatedVersions.push(v));
|
||||
|
||||
it('should stop requesting full page navigations when updates are activated', run(() => {
|
||||
swu.$$availableSubj.next({available: {hash: 'foo'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'bar'}});
|
||||
expect(location.fullPageNavigationNeeded).toHaveBeenCalledTimes(1);
|
||||
|
||||
service.ngOnDestroy();
|
||||
location.fullPageNavigationNeeded.calls.reset();
|
||||
|
||||
swu.$$availableSubj.next({available: {hash: 'baz'}});
|
||||
swu.$$activatedSubj.next({current: {hash: 'qux'}});
|
||||
|
||||
expect(activatedVersions).toEqual(['bar']);
|
||||
expect(location.fullPageNavigationNeeded).not.toHaveBeenCalled();
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
import { ApplicationRef, Injectable, OnDestroy } from '@angular/core';
|
||||
import { SwUpdate } from '@angular/service-worker';
|
||||
import { concat, interval, NEVER, Observable, Subject } from 'rxjs';
|
||||
import { first, map, takeUntil, tap } from 'rxjs/operators';
|
||||
import { concat, interval, Subject } from 'rxjs';
|
||||
import { first, takeUntil, tap } from 'rxjs/operators';
|
||||
|
||||
import { LocationService } from 'app/shared/location.service';
|
||||
import { Logger } from 'app/shared/logger.service';
|
||||
|
||||
|
||||
|
|
@ -19,12 +20,10 @@ export class SwUpdatesService implements OnDestroy {
|
|||
private checkInterval = 1000 * 60 * 60 * 6; // 6 hours
|
||||
private onDestroy = new Subject<void>();
|
||||
|
||||
/** Emit the version hash whenever an update is activated. */
|
||||
updateActivated: Observable<string>;
|
||||
|
||||
constructor(appRef: ApplicationRef, private logger: Logger, private swu: SwUpdate) {
|
||||
constructor(
|
||||
appRef: ApplicationRef, location: LocationService, private logger: Logger,
|
||||
private swu: SwUpdate) {
|
||||
if (!swu.isEnabled) {
|
||||
this.updateActivated = NEVER.pipe(takeUntil(this.onDestroy));
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -45,12 +44,13 @@ export class SwUpdatesService implements OnDestroy {
|
|||
)
|
||||
.subscribe(() => this.swu.activateUpdate());
|
||||
|
||||
// Notify about activated updates.
|
||||
this.updateActivated = this.swu.activated.pipe(
|
||||
// Request a full page navigation once an update has been activated.
|
||||
this.swu.activated
|
||||
.pipe(
|
||||
tap(evt => this.log(`Update activated: ${JSON.stringify(evt)}`)),
|
||||
map(evt => evt.current.hash),
|
||||
takeUntil(this.onDestroy),
|
||||
);
|
||||
)
|
||||
.subscribe(() => location.fullPageNavigationNeeded());
|
||||
}
|
||||
|
||||
ngOnDestroy() {
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ export class MockLocationService {
|
|||
currentPath = this.currentUrl.pipe(map(url => url.match(/[^?#]*/)![0]));
|
||||
search = jasmine.createSpy('search').and.returnValue({});
|
||||
setSearch = jasmine.createSpy('setSearch');
|
||||
fullPageNavigationNeeded = jasmine.createSpy('Location.fullPageNavigationNeeded');
|
||||
go = jasmine.createSpy('Location.go').and
|
||||
.callFake((url: string) => this.urlSubject.next(url));
|
||||
goExternal = jasmine.createSpy('Location.goExternal');
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
"master": {
|
||||
"uncompressed": {
|
||||
"runtime-es2015": 3033,
|
||||
"main-es2015": 448327,
|
||||
"main-es2015": 447565,
|
||||
"polyfills-es2015": 52343
|
||||
}
|
||||
}
|
||||
|
|
@ -12,7 +12,7 @@
|
|||
"master": {
|
||||
"uncompressed": {
|
||||
"runtime-es2015": 3033,
|
||||
"main-es2015": 448538,
|
||||
"main-es2015": 447774,
|
||||
"polyfills-es2015": 52493
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue