From 1a6a79b63a9ca11c0ed1cce84a6cfa901e75b104 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 11 Jun 2021 20:14:34 +0300 Subject: [PATCH] refactor(docs-infra): provide `local-/sessionStorage` via DI (#42259) Previously, we had the same logic in a couple of places to safely access the `Window`'s `local-/sessionStorage` and provide a no-op fallback if necessary. Soon, we will need the same logic for the cookies popup (see #42209). This commit reduces code duplication by providing `local-/sessionStorage` as injectables and sharing the logic for accessing them safely. This also makes it easier to mock the storage in tests without having to mess with the actual `Window` object. NOTE: This commit actually decreases the payload size in the `main` bundle by 40B. PR Close #42259 --- aio/src/app/app.module.ts | 2 + .../notification.component.spec.ts | 31 ++------- .../notification/notification.component.ts | 24 +------ aio/src/app/shared/scroll.service.spec.ts | 40 +++--------- aio/src/app/shared/scroll.service.ts | 20 +----- aio/src/app/shared/storage.service.spec.ts | 37 +++++++++++ aio/src/app/shared/storage.service.ts | 29 +++++++++ .../theme-toggle.component.spec.ts | 64 ++++++++----------- .../theme-picker/theme-toggle.component.ts | 31 ++------- goldens/size-tracking/aio-payloads.json | 4 +- 10 files changed, 123 insertions(+), 159 deletions(-) create mode 100644 aio/src/app/shared/storage.service.spec.ts create mode 100644 aio/src/app/shared/storage.service.ts diff --git a/aio/src/app/app.module.ts b/aio/src/app/app.module.ts index 37c7be96d8..17e8bdba11 100644 --- a/aio/src/app/app.module.ts +++ b/aio/src/app/app.module.ts @@ -21,6 +21,7 @@ import { ModeBannerComponent } from 'app/layout/mode-banner/mode-banner.componen import { GaService } from 'app/shared/ga.service'; import { Logger } from 'app/shared/logger.service'; import { LocationService } from 'app/shared/location.service'; +import { STORAGE_PROVIDERS } from 'app/shared/storage.service'; import { NavigationService } from 'app/navigation/navigation.service'; import { DocumentService } from 'app/documents/document.service'; import { SearchService } from 'app/search/search.service'; @@ -187,6 +188,7 @@ export const svgIconProviders = [ ScrollService, ScrollSpyService, SearchService, + STORAGE_PROVIDERS, svgIconProviders, TocService, { provide: CurrentDateToken, useFactory: currentDateProvider }, diff --git a/aio/src/app/layout/notification/notification.component.spec.ts b/aio/src/app/layout/notification/notification.component.spec.ts index 4cdfbe3ef3..7aa622abda 100644 --- a/aio/src/app/layout/notification/notification.component.spec.ts +++ b/aio/src/app/layout/notification/notification.component.spec.ts @@ -3,8 +3,8 @@ import { Component, NO_ERRORS_SCHEMA } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; import { CurrentDateToken } from 'app/shared/current-date'; +import { LocalStorage, NoopStorage } from 'app/shared/storage.service'; import { NotificationComponent } from './notification.component'; -import { WindowToken } from 'app/shared/window'; describe('NotificationComponent', () => { let component: NotificationComponent; @@ -14,7 +14,7 @@ describe('NotificationComponent', () => { TestBed.configureTestingModule({ declarations: [TestComponent, NotificationComponent], providers: [ - { provide: WindowToken, useClass: MockWindow }, + { provide: LocalStorage, useValue: new NoopStorage() }, { provide: CurrentDateToken, useValue: now }, ], imports: [NoopAnimationsModule], @@ -92,7 +92,8 @@ describe('NotificationComponent', () => { it('should update localStorage key when dismiss is called', () => { configTestingModule(); createComponent(); - const setItemSpy: jasmine.Spy = (TestBed.inject(WindowToken) as MockWindow).localStorage.setItem; + const localStorage = TestBed.inject(LocalStorage); + const setItemSpy = spyOn(localStorage, 'setItem'); component.dismiss(); expect(setItemSpy).toHaveBeenCalledWith('aio-notification/survey-january-2018', 'hide'); }); @@ -105,28 +106,12 @@ describe('NotificationComponent', () => { it('should not show the notification if the there is a "hide" flag in localStorage', () => { configTestingModule(); - const getItemSpy: jasmine.Spy = (TestBed.inject(WindowToken) as MockWindow).localStorage.getItem; - getItemSpy.and.returnValue('hide'); + const localStorage = TestBed.inject(LocalStorage); + const getItemSpy = spyOn(localStorage, 'getItem').and.returnValue('hide'); createComponent(); expect(getItemSpy).toHaveBeenCalledWith('aio-notification/survey-january-2018'); expect(component.showNotification).toBe('hide'); }); - - it('should not break when cookies are disabled in the browser', () => { - configTestingModule(); - - // Simulate `window.localStorage` being inaccessible, when cookies are disabled. - const mockWindow: MockWindow = TestBed.inject(WindowToken); - Object.defineProperty(mockWindow, 'localStorage', { - get() { throw new Error('The operation is insecure'); }, - }); - - expect(() => createComponent()).not.toThrow(); - expect(component.showNotification).toBe('show'); - - component.dismiss(); - expect(component.showNotification).toBe('hide'); - }); }); @Component({ @@ -145,7 +130,3 @@ describe('NotificationComponent', () => { }) class TestComponent { } - -class MockWindow { - localStorage = jasmine.createSpyObj('localStorage', ['getItem', 'setItem']); -} diff --git a/aio/src/app/layout/notification/notification.component.ts b/aio/src/app/layout/notification/notification.component.ts index 868f69e284..f8ce4270d9 100644 --- a/aio/src/app/layout/notification/notification.component.ts +++ b/aio/src/app/layout/notification/notification.component.ts @@ -1,7 +1,7 @@ import { animate, state, style, trigger, transition } from '@angular/animations'; import { Component, EventEmitter, HostBinding, Inject, Input, OnInit, Output } from '@angular/core'; import { CurrentDateToken } from 'app/shared/current-date'; -import { WindowToken } from 'app/shared/window'; +import { LocalStorage } from 'app/shared/storage.service'; const LOCAL_STORAGE_NAMESPACE = 'aio-notification/'; @@ -20,8 +20,6 @@ const LOCAL_STORAGE_NAMESPACE = 'aio-notification/'; ] }) export class NotificationComponent implements OnInit { - private storage: Storage; - @Input() dismissOnContentClick: boolean; @Input() notificationId: string; @Input() expirationDate: string; @@ -30,25 +28,7 @@ export class NotificationComponent implements OnInit { @HostBinding('@hideAnimation') showNotification: 'show'|'hide'; - constructor( - @Inject(WindowToken) window: Window, - @Inject(CurrentDateToken) private currentDate: Date - ) { - try { - this.storage = window.localStorage; - } catch { - // When cookies are disabled in the browser, even trying to access - // `window.localStorage` throws an error. Use a no-op storage. - this.storage = { - length: 0, - clear: () => undefined, - getItem: () => null, - key: () => null, - removeItem: () => undefined, - setItem: () => undefined - }; - } - } + constructor(@Inject(LocalStorage) private storage: Storage, @Inject(CurrentDateToken) private currentDate: Date) {} ngOnInit() { const previouslyHidden = this.storage.getItem(LOCAL_STORAGE_NAMESPACE + this.notificationId) === 'hide'; diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts index 11b32a2bec..7aeaedb78a 100644 --- a/aio/src/app/shared/scroll.service.spec.ts +++ b/aio/src/app/shared/scroll.service.spec.ts @@ -5,6 +5,7 @@ import {Injector} from '@angular/core'; import {fakeAsync, tick} from '@angular/core/testing'; import {ScrollService, topMargin} from './scroll.service'; +import {SessionStorage, NoopStorage} from './storage.service'; describe('ScrollService', () => { const scrollServiceInstances: ScrollService[] = []; @@ -20,6 +21,7 @@ describe('ScrollService', () => { let platformLocation: MockPlatformLocation; let scrollService: ScrollService; let location: SpyLocation; + let sessionStorage: Storage; class MockPlatformLocation { hash: string; @@ -46,13 +48,14 @@ describe('ScrollService', () => { { provide: ScrollService, useFactory: createScrollService, - deps: [DOCUMENT, PlatformLocation, ViewportScroller, Location], + deps: [DOCUMENT, PlatformLocation, ViewportScroller, Location, SessionStorage], }, - {provide: Location, useClass: SpyLocation, deps: [] }, + {provide: Location, useClass: SpyLocation, deps: []}, {provide: DOCUMENT, useClass: MockDocument, deps: []}, {provide: PlatformLocation, useClass: MockPlatformLocation, deps: []}, {provide: ViewportScroller, useValue: viewportScrollerStub}, - {provide: LocationStrategy, useClass: MockLocationStrategy, deps: []} + {provide: LocationStrategy, useClass: MockLocationStrategy, deps: []}, + {provide: SessionStorage, useValue: new NoopStorage()}, ] }); @@ -60,13 +63,13 @@ describe('ScrollService', () => { document = injector.get(DOCUMENT) as unknown as MockDocument; scrollService = injector.get(ScrollService); location = injector.get(Location) as unknown as SpyLocation; + sessionStorage = injector.get(SessionStorage); spyOn(window, 'scrollBy'); }); afterEach(() => { scrollServiceInstances.forEach(instance => instance.ngOnDestroy()); - window.sessionStorage.clear(); }); it('should debounce `updateScrollPositonInHistory()`', fakeAsync(() => { @@ -92,7 +95,8 @@ describe('ScrollService', () => { configurable: true, }); scrollService = createScrollService( - document, platformLocation as PlatformLocation, viewportScrollerStub, location); + document, platformLocation as PlatformLocation, viewportScrollerStub, location, + sessionStorage); expect(scrollService.supportManualScrollRestoration).toBe(false); } finally { @@ -112,32 +116,6 @@ describe('ScrollService', () => { } }); - it('should not break when cookies are disabled in the browser', () => { - expect(() => { - const originalSessionStorage = Object.getOwnPropertyDescriptor(window, 'sessionStorage') as PropertyDescriptor; - - try { - // Simulate `window.sessionStorage` being inaccessible, when cookies are disabled. - Object.defineProperty(window, 'sessionStorage', { - get() { - throw new Error('The operation is insecure'); - }, - }); - - const platformLoc = platformLocation as PlatformLocation; - const service = createScrollService(document, platformLoc, viewportScrollerStub, location); - - service.updateScrollLocationHref(); - expect(service.getStoredScrollLocationHref()).toBeNull(); - - service.removeStoredScrollInfo(); - expect(service.getStoredScrollPosition()).toBeNull(); - } finally { - Object.defineProperty(window, 'sessionStorage', originalSessionStorage); - } - }).not.toThrow(); - }); - describe('#topOffset', () => { it('should query for the top-bar by CSS selector', () => { expect(document.querySelector).not.toHaveBeenCalled(); diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index 0788c3200e..43155f1c98 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -2,6 +2,7 @@ import {DOCUMENT, Location, PlatformLocation, PopStateEvent, ViewportScroller} f import {Inject, Injectable, OnDestroy} from '@angular/core'; import {fromEvent, Subject} from 'rxjs'; import {debounceTime, takeUntil} from 'rxjs/operators'; +import {SessionStorage} from './storage.service'; type ScrollPosition = [number, number]; interface ScrollPositionPopStateEvent extends PopStateEvent { @@ -18,7 +19,6 @@ export class ScrollService implements OnDestroy { private _topOffset: number|null; private _topOfPageElement: Element; private onDestroy = new Subject(); - private storage: Storage; // The scroll position which has to be restored, after a `popstate` event. poppedStateScrollPosition: ScrollPosition|null = null; @@ -45,22 +45,8 @@ export class ScrollService implements OnDestroy { constructor( @Inject(DOCUMENT) private document: any, private platformLocation: PlatformLocation, - private viewportScroller: ViewportScroller, private location: Location) { - try { - this.storage = window.sessionStorage; - } catch { - // When cookies are disabled in the browser, even trying to access - // `window.sessionStorage` throws an error. Use a no-op storage. - this.storage = { - length: 0, - clear: () => undefined, - getItem: () => null, - key: () => null, - removeItem: () => undefined, - setItem: () => undefined - }; - } - + private viewportScroller: ViewportScroller, private location: Location, + @Inject(SessionStorage) private storage: Storage) { // On resize, the toolbar might change height, so "invalidate" the top offset. fromEvent(window, 'resize') .pipe(takeUntil(this.onDestroy)) diff --git a/aio/src/app/shared/storage.service.spec.ts b/aio/src/app/shared/storage.service.spec.ts new file mode 100644 index 0000000000..5f39dd6bf9 --- /dev/null +++ b/aio/src/app/shared/storage.service.spec.ts @@ -0,0 +1,37 @@ +import { Injector } from '@angular/core'; +import { LocalStorage, NoopStorage, SessionStorage, STORAGE_PROVIDERS } from './storage.service'; +import { WindowToken } from './window'; + +[ + ['localStorage', LocalStorage] as const, + ['sessionStorage', SessionStorage] as const, +].forEach(([storagePropName, storageToken]) => { + let getStorageSpy: jasmine.Spy; + let injector: Injector; + + beforeEach(() => { + getStorageSpy = jasmine.createSpy(`get ${storagePropName}`); + injector = Injector.create({ + providers: [ + STORAGE_PROVIDERS, + { + provide: WindowToken, + useValue: Object.defineProperty({}, storagePropName, { get: getStorageSpy }), + }, + ], + }); + }); + + it('should return the storage from `window`', () => { + const mockStorage = { mock: true } as unknown as Storage; + getStorageSpy.and.returnValue(mockStorage); + + expect(injector.get(storageToken)).toBe(mockStorage); + }); + + it('should return a no-op storage if accessing the storage on `window` errors', () => { + getStorageSpy.and.throwError('Can\'t touch this!'); + + expect(injector.get(storageToken)).toBeInstanceOf(NoopStorage); + }); +}); diff --git a/aio/src/app/shared/storage.service.ts b/aio/src/app/shared/storage.service.ts new file mode 100644 index 0000000000..e96f90da21 --- /dev/null +++ b/aio/src/app/shared/storage.service.ts @@ -0,0 +1,29 @@ +import { InjectionToken, StaticProvider } from '@angular/core'; +import { WindowToken } from './window'; + +export const LocalStorage = new InjectionToken('LocalStorage'); +export const SessionStorage = new InjectionToken('SessionStorage'); + +export const STORAGE_PROVIDERS: StaticProvider[] = [ + { provide: LocalStorage, useFactory: (win: Window) => getStorage(win, 'localStorage'), deps: [WindowToken] }, + { provide: SessionStorage, useFactory: (win: Window) => getStorage(win, 'sessionStorage'), deps: [WindowToken] }, +]; + +export class NoopStorage implements Storage { + length = 0; + clear() {} + getItem() { return null; } + key() { return null; } + removeItem() {} + setItem() {} +} + +function getStorage(win: Window, storageType: 'localStorage' | 'sessionStorage'): Storage { + // When cookies are disabled in the browser, even trying to access `window[storageType]` throws an + // error. If so, return a no-op storage. + try { + return win[storageType]; + } catch { + return new NoopStorage(); + } +} diff --git a/aio/src/app/shared/theme-picker/theme-toggle.component.spec.ts b/aio/src/app/shared/theme-picker/theme-toggle.component.spec.ts index 9a63fa20a8..60cf965a00 100644 --- a/aio/src/app/shared/theme-picker/theme-toggle.component.spec.ts +++ b/aio/src/app/shared/theme-picker/theme-toggle.component.spec.ts @@ -1,43 +1,8 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { MatIconModule } from '@angular/material/icon'; import { By } from '@angular/platform-browser'; -import { ThemeStorage, ThemeToggleComponent } from './theme-toggle.component'; - -class FakeThemeStorage implements ThemeStorage { - fakeStorage: string | null = null; - - getThemePreference(): string | null { - return this.fakeStorage; - } - - setThemePreference(isDark: boolean): void { - this.fakeStorage = String(isDark); - } -} - -// Verify that FakeThemeStorage behaves like ThemeStorage would -describe('FakeThemeStorage', () => { - let themeStorage: ThemeStorage; - - beforeEach(() => { - themeStorage = new FakeThemeStorage(); - }); - - it('should have null stored initially', () => { - expect(themeStorage.getThemePreference()).toBeNull(); - }); - - it('should store true as a string if isDark is true', () => { - themeStorage.setThemePreference(true); - expect(themeStorage.getThemePreference()).toBe('true'); - }); - - it('should store false as a string if isDark is false', () => { - themeStorage.setThemePreference(false); - expect(themeStorage.getThemePreference()).toBe('false'); - }); -}); - +import { LocalStorage, NoopStorage } from '../storage.service'; +import { storageKey as themeStorageKey, ThemeToggleComponent } from './theme-toggle.component'; describe('ThemeToggleComponent', () => { let component: ThemeToggleComponent; @@ -47,6 +12,7 @@ describe('ThemeToggleComponent', () => { TestBed.configureTestingModule({ declarations: [ ThemeToggleComponent ], imports: [ MatIconModule ], + providers: [ { provide: LocalStorage, useValue: new NoopStorage() } ], }); fixture = TestBed.createComponent(ThemeToggleComponent); @@ -82,6 +48,30 @@ describe('ThemeToggleComponent', () => { expect(component.getToggleLabel()).toBe('Switch to light mode'); }); + it('should store the theme in `localStorage`', () => { + const storage = TestBed.inject(LocalStorage); + const setItemSpy = spyOn(storage, 'setItem'); + + component.toggleTheme(); + expect(setItemSpy).toHaveBeenCalledWith(themeStorageKey, 'true'); + + component.toggleTheme(); + expect(setItemSpy).toHaveBeenCalledWith(themeStorageKey, 'false'); + }); + + it('should initialize the theme from `localStorage`', () => { + const storage = TestBed.inject(LocalStorage); + const getItemSpy = spyOn(storage, 'getItem').withArgs(themeStorageKey); + + getItemSpy.and.returnValue('false'); + const component1 = TestBed.createComponent(ThemeToggleComponent).componentInstance; + expect(component1.isDark).toBe(false); + + getItemSpy.and.returnValue('true'); + const component2 = TestBed.createComponent(ThemeToggleComponent).componentInstance; + expect(component2.isDark).toBe(true); + }); + // Helpers function getToggleButton(): HTMLButtonElement { return fixture.debugElement.query(By.css('button')).nativeElement; diff --git a/aio/src/app/shared/theme-picker/theme-toggle.component.ts b/aio/src/app/shared/theme-picker/theme-toggle.component.ts index bb05fff53f..daabcd127b 100644 --- a/aio/src/app/shared/theme-picker/theme-toggle.component.ts +++ b/aio/src/app/shared/theme-picker/theme-toggle.component.ts @@ -1,27 +1,8 @@ import { DOCUMENT } from '@angular/common'; -import { Component, Inject, Injectable } from '@angular/core'; +import { Component, Inject } from '@angular/core'; +import { LocalStorage } from 'app/shared/storage.service'; -/** Injectable facade around localStorage for theme preference to make testing easier. */ -@Injectable({ providedIn: 'root' }) -export class ThemeStorage { - getThemePreference(): string | null { - // Wrap localStorage access in try/catch because user agents can block localStorage. If it is - // blocked, we treat it as if no preference was previously stored. - try { - return localStorage.getItem('aio-theme'); - } catch { - return null; - } - } - - setThemePreference(isDark: boolean): void { - // Wrap localStorage access in try/catch because user agents can block localStorage. If it - // fails, we persist nothing. - try { - localStorage.setItem('aio-theme', String(isDark)); - } catch { } - } -} +export const storageKey = 'aio-theme'; @Component({ selector: 'aio-theme-toggle', @@ -37,7 +18,7 @@ export class ThemeStorage { export class ThemeToggleComponent { isDark = false; - constructor(@Inject(DOCUMENT) private document: Document, private readonly themeStorage: ThemeStorage) { + constructor(@Inject(DOCUMENT) private document: Document, @Inject(LocalStorage) private storage: Storage) { this.initializeThemeFromPreferences(); } @@ -48,7 +29,7 @@ export class ThemeToggleComponent { private initializeThemeFromPreferences(): void { // Check whether there's an explicit preference in localStorage. - const storedPreference = this.themeStorage.getThemePreference(); + const storedPreference = this.storage.getItem(storageKey); // If we do have a preference in localStorage, use that. Otherwise, // initialize based on the prefers-color-scheme media query. @@ -86,6 +67,6 @@ export class ThemeToggleComponent { customLinkElement.href = `${this.getThemeName()}-theme.css`; } - this.themeStorage.setThemePreference(this.isDark); + this.storage.setItem(storageKey, String(this.isDark)); } } diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index 08cd0c6561..fa92ee0f9c 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime-es2017": 4619, - "main-es2017": 454043, + "main-es2017": 454003, "polyfills-es2017": 55210 } } @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2017": 4619, - "main-es2017": 454178, + "main-es2017": 454138, "polyfills-es2017": 55348 } }