From 4d11fcf683cdf9cc3fcf33eccb4c9c5c80402200 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Thu, 30 Sep 2021 15:47:02 +0200 Subject: [PATCH] Review feedback. Signed-off-by: Eric Peterson --- .changeset/analytics-sings-stormy-weather.md | 2 +- .github/styles/vocab.txt | 1 - packages/core-app-api/api-report.md | 10 ++++++++ .../AnalyticsApi/NoOpAnalyticsApi.ts | 23 +++++++++++++++++++ .../implementations/AnalyticsApi/index.ts | 17 ++++++++++++++ .../src/apis/implementations/index.ts | 1 + packages/core-app-api/src/app/App.test.tsx | 11 ++++++--- packages/core-app-api/src/app/defaultApis.ts | 3 +++ .../core-app-api/src/routing/RouteTracker.tsx | 2 ++ .../src/analytics/AnalyticsContext.tsx | 14 +++++------ .../src/analytics/useAnalytics.test.tsx | 11 --------- .../src/analytics/useAnalytics.tsx | 21 ++++------------- .../src/extensions/extensions.tsx | 2 ++ plugins/analytics-module-ga/README.md | 5 +--- plugins/analytics-module-ga/config.d.ts | 1 - 15 files changed, 78 insertions(+), 46 deletions(-) create mode 100644 packages/core-app-api/src/apis/implementations/AnalyticsApi/NoOpAnalyticsApi.ts create mode 100644 packages/core-app-api/src/apis/implementations/AnalyticsApi/index.ts diff --git a/.changeset/analytics-sings-stormy-weather.md b/.changeset/analytics-sings-stormy-weather.md index 4b5eb1f237..f3e6becdf5 100644 --- a/.changeset/analytics-sings-stormy-weather.md +++ b/.changeset/analytics-sings-stormy-weather.md @@ -21,4 +21,4 @@ analytics context, which can be useful for analyzing plugin usage: ``` These events can be identified and handled by checking for the action -`navigate` and the componentName `App`. +`navigate` and the extension `App`. diff --git a/.github/styles/vocab.txt b/.github/styles/vocab.txt index 7508ab8033..a38baab3dc 100644 --- a/.github/styles/vocab.txt +++ b/.github/styles/vocab.txt @@ -88,7 +88,6 @@ firehydrant FireHydrant Firekube Fiverr -ga gitbeaker GitHub GitLab diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 4b804c2cc2..2f057b329f 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -5,6 +5,8 @@ ```ts import { AlertApi } from '@backstage/core-plugin-api'; import { AlertMessage } from '@backstage/core-plugin-api'; +import { AnalyticsApi } from '@backstage/core-plugin-api'; +import { AnalyticsEvent } from '@backstage/core-plugin-api'; import { AnyApiFactory } from '@backstage/core-plugin-api'; import { AnyApiRef } from '@backstage/core-plugin-api'; import { ApiFactory } from '@backstage/core-plugin-api'; @@ -456,6 +458,14 @@ export class MicrosoftAuth { }: OAuthApiCreateOptions): typeof microsoftAuthApiRef.T; } +// Warning: (ae-missing-release-tag) "NoOpAnalyticsApi" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export class NoOpAnalyticsApi implements AnalyticsApi { + // (undocumented) + captureEvent(_event: AnalyticsEvent): void; +} + // Warning: (ae-missing-release-tag) "OAuth2" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/packages/core-app-api/src/apis/implementations/AnalyticsApi/NoOpAnalyticsApi.ts b/packages/core-app-api/src/apis/implementations/AnalyticsApi/NoOpAnalyticsApi.ts new file mode 100644 index 0000000000..a3e45006ba --- /dev/null +++ b/packages/core-app-api/src/apis/implementations/AnalyticsApi/NoOpAnalyticsApi.ts @@ -0,0 +1,23 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { AnalyticsApi, AnalyticsEvent } from '@backstage/core-plugin-api'; + +/** + * Base implementation for the AnalyticsApi that does nothing. + */ +export class NoOpAnalyticsApi implements AnalyticsApi { + captureEvent(_event: AnalyticsEvent): void {} +} diff --git a/packages/core-app-api/src/apis/implementations/AnalyticsApi/index.ts b/packages/core-app-api/src/apis/implementations/AnalyticsApi/index.ts new file mode 100644 index 0000000000..6bb27dd6d0 --- /dev/null +++ b/packages/core-app-api/src/apis/implementations/AnalyticsApi/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { NoOpAnalyticsApi } from './NoOpAnalyticsApi'; diff --git a/packages/core-app-api/src/apis/implementations/index.ts b/packages/core-app-api/src/apis/implementations/index.ts index 31f01f2bff..c494f966f2 100644 --- a/packages/core-app-api/src/apis/implementations/index.ts +++ b/packages/core-app-api/src/apis/implementations/index.ts @@ -21,6 +21,7 @@ export * from './auth'; export * from './AlertApi'; +export * from './AnalyticsApi'; export * from './AppThemeApi'; export * from './ConfigApi'; export * from './DiscoveryApi'; diff --git a/packages/core-app-api/src/app/App.test.tsx b/packages/core-app-api/src/app/App.test.tsx index cd323fdb6e..7eb4220876 100644 --- a/packages/core-app-api/src/app/App.test.tsx +++ b/packages/core-app-api/src/app/App.test.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import { LocalStorageFeatureFlags } from '../apis'; +import { LocalStorageFeatureFlags, NoOpAnalyticsApi } from '../apis'; import { MockAnalyticsApi, renderWithEffects, @@ -63,6 +63,10 @@ describe('generateBoundRoutes', () => { }); describe('Integration Test', () => { + const noOpAnalyticsApi = createApiFactory( + analyticsApiRef, + new NoOpAnalyticsApi(), + ); const plugin1RouteRef = createRouteRef({ id: 'ref-1' }); const plugin1RouteRef2 = createRouteRef({ id: 'ref-1-2' }); const plugin2RouteRef = createRouteRef({ id: 'ref-2', params: ['x'] }); @@ -182,7 +186,7 @@ describe('Integration Test', () => { it('runs happy paths', async () => { const app = new PrivateAppImpl({ - apis: [], + apis: [noOpAnalyticsApi], defaultApis: [], themes: [ { @@ -236,7 +240,7 @@ describe('Integration Test', () => { it('runs happy paths without optional routes', async () => { const app = new PrivateAppImpl({ - apis: [], + apis: [noOpAnalyticsApi], defaultApis: [], themes: [ { @@ -282,6 +286,7 @@ describe('Integration Test', () => { jest.spyOn(storageFlags, 'registerFlag'); const apis = [ + noOpAnalyticsApi, createApiFactory({ api: featureFlagsApiRef, deps: { configApi: configApiRef }, diff --git a/packages/core-app-api/src/app/defaultApis.ts b/packages/core-app-api/src/app/defaultApis.ts index 2322f72e8d..d0f7786b4c 100644 --- a/packages/core-app-api/src/app/defaultApis.ts +++ b/packages/core-app-api/src/app/defaultApis.ts @@ -16,6 +16,7 @@ import { AlertApiForwarder, + NoOpAnalyticsApi, ErrorApiForwarder, ErrorAlerter, GoogleAuth, @@ -36,6 +37,7 @@ import { import { createApiFactory, alertApiRef, + analyticsApiRef, errorApiRef, discoveryApiRef, oauthRequestApiRef, @@ -65,6 +67,7 @@ export const defaultApis = [ ), }), createApiFactory(alertApiRef, new AlertApiForwarder()), + createApiFactory(analyticsApiRef, new NoOpAnalyticsApi()), createApiFactory({ api: errorApiRef, deps: { alertApi: alertApiRef }, diff --git a/packages/core-app-api/src/routing/RouteTracker.tsx b/packages/core-app-api/src/routing/RouteTracker.tsx index 464f451c9c..b745dd9235 100644 --- a/packages/core-app-api/src/routing/RouteTracker.tsx +++ b/packages/core-app-api/src/routing/RouteTracker.tsx @@ -99,6 +99,8 @@ const TrackNavigation = ({ */ export const RouteTracker = ({ tree }: { tree: React.ReactNode }) => { const { pathname, search, hash } = useLocation(); + // todo(iamEAP): Work this into the existing traversal and make the data + // available on the provider. Then grab from app instance on the router. const { routeObjects } = useMemo(() => { return traverseElementTree({ root: tree, diff --git a/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx b/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx index 284b4186b2..f097fd577f 100644 --- a/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx +++ b/packages/core-plugin-api/src/analytics/AnalyticsContext.tsx @@ -14,9 +14,10 @@ * limitations under the License. */ -import React, { createContext, ReactNode, useContext, useMemo } from 'react'; +import React, { createContext, ReactNode, useContext } from 'react'; import { AnalyticsContextValue } from './types'; +// todo(iamEAP): Manage this using a version bridge. const AnalyticsReactContext = createContext({ routeRef: 'unknown', pluginId: 'root', @@ -47,13 +48,10 @@ export const AnalyticsContext = ({ children: ReactNode; }) => { const parentValues = useAnalyticsContext(); - const combinedValue = useMemo( - () => ({ - ...parentValues, - ...attributes, - }), - [parentValues, attributes], - ); + const combinedValue = { + ...parentValues, + ...attributes, + }; return ( diff --git a/packages/core-plugin-api/src/analytics/useAnalytics.test.tsx b/packages/core-plugin-api/src/analytics/useAnalytics.test.tsx index 22021026af..3c932c02f7 100644 --- a/packages/core-plugin-api/src/analytics/useAnalytics.test.tsx +++ b/packages/core-plugin-api/src/analytics/useAnalytics.test.tsx @@ -23,17 +23,6 @@ jest.mock('../apis'); const mocked = (f: Function) => f as jest.Mock; describe('useAnalytics', () => { - it('returns tracker with no implementation defined', () => { - // Simulate useApi() throwing an error. - mocked(useApi).mockImplementation(() => { - throw new Error(); - }); - - // Result should still have a captureEvent method. - const { result } = renderHook(() => useAnalytics()); - expect(result.current.captureEvent).toBeDefined(); - }); - it('returns tracker from defined analytics api', () => { const captureEvent = jest.fn(); diff --git a/packages/core-plugin-api/src/analytics/useAnalytics.tsx b/packages/core-plugin-api/src/analytics/useAnalytics.tsx index 2dcb284879..c3a1ae3b9c 100644 --- a/packages/core-plugin-api/src/analytics/useAnalytics.tsx +++ b/packages/core-plugin-api/src/analytics/useAnalytics.tsx @@ -19,7 +19,10 @@ import { analyticsApiRef, AnalyticsTracker, useApi } from '../apis'; import { useRef } from 'react'; import { Tracker } from './Tracker'; -function useTracker(): AnalyticsTracker { +/** + * Get a pre-configured analytics tracker. + */ +export function useAnalytics(): AnalyticsTracker { const trackerRef = useRef(null); const analyticsApi = useApi(analyticsApiRef); const context = useAnalyticsContext(); @@ -36,19 +39,3 @@ function useTracker(): AnalyticsTracker { return tracker; } - -/** - * Get a pre-configured analytics tracker. - */ -export function useAnalytics(): AnalyticsTracker { - // Return a no-op tracker if no implementation for the Analytics API is - // available. Having no default Analytics API implementation enables simple - // provider installation via plugin instantiation. - try { - return useTracker(); - } catch { - return { - captureEvent: () => {}, - }; - } -} diff --git a/packages/core-plugin-api/src/extensions/extensions.tsx b/packages/core-plugin-api/src/extensions/extensions.tsx index 5427b5f61c..4aff027f31 100644 --- a/packages/core-plugin-api/src/extensions/extensions.tsx +++ b/packages/core-plugin-api/src/extensions/extensions.tsx @@ -133,6 +133,8 @@ export function createReactExtension< const Result: any = (props: any) => { const app = useApp(); const { Progress } = app.getComponents(); + // todo(iamEAP): Account for situations where this is attached via + // separate calls to attachComponentData(). const mountPoint = data?.['core.mountPoint'] as | { id?: string } | undefined; diff --git a/plugins/analytics-module-ga/README.md b/plugins/analytics-module-ga/README.md index 15ad4f1948..a0c4fe517a 100644 --- a/plugins/analytics-module-ga/README.md +++ b/plugins/analytics-module-ga/README.md @@ -36,7 +36,6 @@ events to GA. All that's needed is your Universal Analytics tracking ID: # app-config.yaml app: analytics: - provider: ga ga: trackingId: UA-0000000-0 ``` @@ -58,7 +57,6 @@ to capture Plugin IDs associated with events, including page views. ```yaml app: analytics: - provider: ga ga: trackingId: UA-0000000-0 customDimensionsMetrics: @@ -122,7 +120,7 @@ make and test changes is to do the following: the monorepo and add config for this plugin (see below) 4. Enter this plugin's working directory: `cd plugins/analytics-provider-ga` 5. Start the plugin in isolation: `yarn start` -6. Navigate to the playground page at [/ga](http://localhost:3000/ga) +6. Navigate to the playground page at `http://localhost:3000/ga` 7. Open the web console to see events fire when you navigate or when you interact with instrumented components. @@ -136,7 +134,6 @@ Paste this into your `app-config.local.yaml` while developing this plugin: ```yaml app: analytics: - provider: ga ga: trackingId: UA-0000000-0 debug: true diff --git a/plugins/analytics-module-ga/config.d.ts b/plugins/analytics-module-ga/config.d.ts index 161776e9dc..4ccd66e1dd 100644 --- a/plugins/analytics-module-ga/config.d.ts +++ b/plugins/analytics-module-ga/config.d.ts @@ -20,7 +20,6 @@ export interface Config { // context of the monorepo is too strict. Ideally, this would be marked as // required. analytics?: { - provider: 'ga'; ga: { /** * Google Analytics tracking ID, e.g. UA-000000-0