diff --git a/aio/src/app/shared/location.service.spec.ts b/aio/src/app/shared/location.service.spec.ts index 9661758493..eaf41d1af7 100644 --- a/aio/src/app/shared/location.service.spec.ts +++ b/aio/src/app/shared/location.service.spec.ts @@ -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(); -} - class MockScrollService { removeStoredScrollInfo() { } } diff --git a/aio/src/app/shared/location.service.ts b/aio/src/app/shared/location.service.ts index 463eb54651..b1867dbbfe 100644 --- a/aio/src/app/shared/location.service.ts +++ b/aio/src/app/shared/location.service.ts @@ -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(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 diff --git a/aio/src/app/sw-updates/sw-updates.service.spec.ts b/aio/src/app/sw-updates/sw-updates.service.spec.ts index b833dfbe6d..ee1d0dd1d7 100644 --- a/aio/src/app/sw-updates/sw-updates.service.spec.ts +++ b/aio/src/app/sw-updates/sw-updates.service.spec.ts @@ -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(); })); }); }); diff --git a/aio/src/app/sw-updates/sw-updates.service.ts b/aio/src/app/sw-updates/sw-updates.service.ts index 38ea8c4658..31763f2031 100644 --- a/aio/src/app/sw-updates/sw-updates.service.ts +++ b/aio/src/app/sw-updates/sw-updates.service.ts @@ -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(); - /** Emit the version hash whenever an update is activated. */ - updateActivated: Observable; - - 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( - tap(evt => this.log(`Update activated: ${JSON.stringify(evt)}`)), - map(evt => evt.current.hash), - takeUntil(this.onDestroy), - ); + // Request a full page navigation once an update has been activated. + this.swu.activated + .pipe( + tap(evt => this.log(`Update activated: ${JSON.stringify(evt)}`)), + takeUntil(this.onDestroy), + ) + .subscribe(() => location.fullPageNavigationNeeded()); } ngOnDestroy() { diff --git a/aio/src/testing/location.service.ts b/aio/src/testing/location.service.ts index f51ff294dc..7f73181466 100644 --- a/aio/src/testing/location.service.ts +++ b/aio/src/testing/location.service.ts @@ -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'); diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index c9351b7e58..e76a6780a3 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -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 } }