From 8a57ffc0fa9c1c57d21439d8c28d4abc98b2db54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 8 Sep 2022 09:20:22 +0200 Subject: [PATCH] Ensure that we return stable observer references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/sixty-apes-wait.md | 5 + packages/core-app-api/api-report.md | 5 +- .../StorageApi/UserSettingsStorage.test.ts | 9 +- .../StorageApi/UserSettingsStorage.ts | 118 +++++++++++------- .../implementations/StorageApi/WebStorage.ts | 4 +- .../shortcuts/src/api/LocalStoredShortcuts.ts | 12 +- plugins/user-settings-backend/README.md | 9 +- 7 files changed, 104 insertions(+), 58 deletions(-) create mode 100644 .changeset/sixty-apes-wait.md diff --git a/.changeset/sixty-apes-wait.md b/.changeset/sixty-apes-wait.md new file mode 100644 index 0000000000..1273307519 --- /dev/null +++ b/.changeset/sixty-apes-wait.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-shortcuts': patch +--- + +Ensure that a stable observable is used in the shortcuts API diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 90e18df723..39e5388d68 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -550,6 +550,7 @@ export class UserSettingsStorage implements StorageApi { fetchApi: FetchApi; discoveryApi: DiscoveryApi; errorApi: ErrorApi; + identityApi: IdentityApi; namespace?: string; }): UserSettingsStorage; // (undocumented) @@ -579,7 +580,9 @@ export class WebStorage implements StorageApi { // (undocumented) get(key: string): T | undefined; // (undocumented) - observe$(key: string): Observable>; + observe$( + key: string, + ): Observable>; // (undocumented) remove(key: string): Promise; // (undocumented) diff --git a/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts b/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts index d8ea87b38d..0ebd96d19c 100644 --- a/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts +++ b/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts @@ -18,6 +18,7 @@ import { DiscoveryApi, ErrorApi, FetchApi, + IdentityApi, StorageApi, } from '@backstage/core-plugin-api'; import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils'; @@ -31,7 +32,12 @@ describe('Persistent Storage API', () => { const mockBaseUrl = 'http://backstage:9191/api'; const mockErrorApi = { post: jest.fn(), error$: jest.fn() }; - const mockDiscoveryApi = { getBaseUrl: async () => mockBaseUrl }; + const mockDiscoveryApi = { + getBaseUrl: async () => mockBaseUrl, + }; + const mockIdentityApi: Partial = { + getCredentials: async () => ({ token: 'a-token' }), + }; const createPersistentStorage = ( args?: Partial<{ @@ -45,6 +51,7 @@ describe('Persistent Storage API', () => { errorApi: mockErrorApi, fetchApi: new MockFetchApi(), discoveryApi: mockDiscoveryApi, + identityApi: mockIdentityApi as IdentityApi, ...args, }); }; diff --git a/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts b/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts index d01d5021ee..b595c524cf 100644 --- a/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts +++ b/packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts @@ -18,12 +18,19 @@ import { DiscoveryApi, ErrorApi, FetchApi, + IdentityApi, StorageApi, StorageValueSnapshot, } from '@backstage/core-plugin-api'; import { ResponseError } from '@backstage/errors'; import { JsonValue, Observable } from '@backstage/types'; import ObservableImpl from 'zen-observable'; +import { WebStorage } from './WebStorage'; + +const JSON_HEADERS = { + 'Content-Type': 'application/json; charset=utf-8', + Accept: 'application/json', +}; const buckets = new Map(); @@ -38,26 +45,25 @@ export class UserSettingsStorage implements StorageApi { ZenObservable.SubscriptionObserver> >(); - private readonly observable = new ObservableImpl< - StorageValueSnapshot - >(subscriber => { - this.subscribers.add(subscriber); - return () => { - this.subscribers.delete(subscriber); - }; - }); + private readonly observables = new Map< + string, + Observable> + >(); private constructor( private readonly namespace: string, private readonly fetchApi: FetchApi, private readonly discoveryApi: DiscoveryApi, private readonly errorApi: ErrorApi, + private readonly identityApi: IdentityApi, + private readonly fallback: WebStorage, ) {} static create(options: { fetchApi: FetchApi; discoveryApi: DiscoveryApi; errorApi: ErrorApi; + identityApi: IdentityApi; namespace?: string; }): UserSettingsStorage { return new UserSettingsStorage( @@ -65,6 +71,11 @@ export class UserSettingsStorage implements StorageApi { options.fetchApi, options.discoveryApi, options.errorApi, + options.identityApi, + WebStorage.create({ + namespace: options.namespace, + errorApi: options.errorApi, + }), ); } @@ -80,6 +91,8 @@ export class UserSettingsStorage implements StorageApi { this.fetchApi, this.discoveryApi, this.errorApi, + this.identityApi, + this.fallback, ), ); } @@ -95,72 +108,81 @@ export class UserSettingsStorage implements StorageApi { }); if (!response.ok && response.status !== 404) { - throw ResponseError.fromResponse(response); + throw await ResponseError.fromResponse(response); } - this.notifyChanges({ - key, - presence: 'absent', - }); + this.notifyChanges({ key, presence: 'absent' }); } async set(key: string, data: T): Promise { + if (!(await this.isSignedIn())) { + await this.fallback.set(key, data); + this.notifyChanges({ key, presence: 'present', value: data }); + } + const fetchUrl = await this.getFetchUrl(key); - const body = JSON.stringify({ value: data }); const response = await this.fetchApi.fetch(fetchUrl, { method: 'PUT', - body, + headers: JSON_HEADERS, + body: JSON.stringify({ value: data }), }); if (!response.ok) { - throw ResponseError.fromResponse(response); + throw await ResponseError.fromResponse(response); } const { value } = await response.json(); - this.notifyChanges({ - key, - value, - presence: 'present', - }); + this.notifyChanges({ key, value, presence: 'present' }); } observe$( key: string, ): Observable> { - // TODO(freben): Introduce server polling or similar, to ensure that different devices update when values change - return this.observable.filter(({ key: messageKey }) => messageKey === key); + if (!this.observables.has(key)) { + this.observables.set( + key, + new ObservableImpl>(subscriber => { + this.subscribers.add(subscriber); + + // TODO(freben): Introduce server polling or similar, to ensure that different devices update when values change + Promise.resolve() + .then(() => this.get(key)) + .then(snapshot => subscriber.next(snapshot)) + .catch(error => this.errorApi.post(error)); + + return () => { + this.subscribers.delete(subscriber); + }; + }).filter(({ key: messageKey }) => messageKey === key), + ); + } + + return this.observables.get(key) as Observable>; } snapshot(key: string): StorageValueSnapshot { - // trigger a reload, ensure it happens on the next tick (after returning) - Promise.resolve() - .then(() => this.get(key)) - .then(snapshot => this.notifyChanges(snapshot)) - .catch(error => this.errorApi.post(error)); - - return { - key, - presence: 'unknown', - }; + return { key, presence: 'unknown' }; } private async get( key: string, ): Promise> { + if (!(await this.isSignedIn())) { + // This explicitly uses WebStorage, which we know is synchronous and doesn't return presence: unknown + return this.fallback.snapshot(key); + } + const fetchUrl = await this.getFetchUrl(key); const response = await this.fetchApi.fetch(fetchUrl); if (response.status === 404) { - return { - key, - presence: 'absent', - }; + return { key, presence: 'absent' }; } if (!response.ok) { - throw ResponseError.fromResponse(response); + throw await ResponseError.fromResponse(response); } try { @@ -172,17 +194,10 @@ export class UserSettingsStorage implements StorageApi { return val; }); - return { - key, - presence: 'present', - value, - }; + return { key, presence: 'present', value }; } catch { // If the value is not valid JSON, we return an unknown presence. This should never happen - return { - key, - presence: 'absent', - }; + return { key, presence: 'absent' }; } } @@ -204,4 +219,13 @@ export class UserSettingsStorage implements StorageApi { } } } + + private async isSignedIn(): Promise { + try { + const credentials = await this.identityApi.getCredentials(); + return credentials?.token ? true : false; + } catch { + return false; + } + } } diff --git a/packages/core-app-api/src/apis/implementations/StorageApi/WebStorage.ts b/packages/core-app-api/src/apis/implementations/StorageApi/WebStorage.ts index ac3d3f20f2..b0cbfbd883 100644 --- a/packages/core-app-api/src/apis/implementations/StorageApi/WebStorage.ts +++ b/packages/core-app-api/src/apis/implementations/StorageApi/WebStorage.ts @@ -86,7 +86,9 @@ export class WebStorage implements StorageApi { this.notifyChanges(key); } - observe$(key: string): Observable> { + observe$( + key: string, + ): Observable> { return this.observable.filter(({ key: messageKey }) => messageKey === key); } diff --git a/plugins/shortcuts/src/api/LocalStoredShortcuts.ts b/plugins/shortcuts/src/api/LocalStoredShortcuts.ts index 9f3561be67..65aeb86e3f 100644 --- a/plugins/shortcuts/src/api/LocalStoredShortcuts.ts +++ b/plugins/shortcuts/src/api/LocalStoredShortcuts.ts @@ -27,12 +27,16 @@ import Observable from 'zen-observable'; * @public */ export class LocalStoredShortcuts implements ShortcutApi { - constructor(private readonly storageApi: StorageApi) {} + private readonly shortcuts: Observable; + + constructor(private readonly storageApi: StorageApi) { + this.shortcuts = Observable.from( + this.storageApi.observe$('items'), + ).map(snapshot => snapshot.value ?? []); + } shortcut$() { - return Observable.from(this.storageApi.observe$('items')).map( - snapshot => snapshot.value ?? [], - ); + return this.shortcuts; } get() { diff --git a/plugins/user-settings-backend/README.md b/plugins/user-settings-backend/README.md index 5a98c86326..45e06731c9 100644 --- a/plugins/user-settings-backend/README.md +++ b/plugins/user-settings-backend/README.md @@ -35,7 +35,7 @@ export default async function createPlugin(env: PluginEnvironment) { // packages/backend/src/index.ts +import userSettings from './plugins/userSettings'; async function main() { -+ const userSettingsEnv = useHotMemoize(module, () => createEnv('userSettings')); ++ const userSettingsEnv = useHotMemoize(module, () => createEnv('user-settings')); const apiRouter = Router(); + apiRouter.use('/user-settings', await userSettings(userSettingsEnv)); } @@ -52,8 +52,9 @@ To make use of the user settings backend, replace the `WebStorage` with the AnyApiFactory, createApiFactory, + discoveryApiRef, -+ fetchApiRef ++ fetchApiRef, errorApiRef, ++ identityApi, + storageApiRef, } from '@backstage/core-plugin-api'; +import { UserSettingsStorage } from '@backstage/core-app-api'; @@ -65,9 +66,9 @@ To make use of the user settings backend, replace the `WebStorage` with the + discoveryApi: discoveryApiRef, + errorApi: errorApiRef, + fetchApi: fetchApiRef, ++ identityApi: identityApiRef + }, -+ factory: ({ discoveryApi, errorApi, fetchApi }) => -+ UserSettingsStorage.create({ discoveryApi, errorApi, fetchApi }), ++ factory: deps => UserSettingsStorage.create(deps), + }), ]; ```