Review feedback.

Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
2021-09-30 15:47:02 +02:00
parent 0da4df22a3
commit 4d11fcf683
15 changed files with 78 additions and 46 deletions
@@ -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`.
@@ -88,7 +88,6 @@ firehydrant
 FireHydrant
 Firekube
 Fiverr
-ga
 gitbeaker
 GitHub
 GitLab
@@ -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)
@@ -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 {}
+}
@@ -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';
@@ -21,6 +21,7 @@
 export * from './auth';

 export * from './AlertApi';
+export * from './AnalyticsApi';
 export * from './AppThemeApi';
 export * from './ConfigApi';
 export * from './DiscoveryApi';
@@ -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 },
@@ -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 },
@@ -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,
@@ -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<AnalyticsContextValue>({
  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 (
    <AnalyticsReactContext.Provider value={combinedValue}>
@@ -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();

@@ -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<Tracker | null>(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: () => {},
-    };
-  }
-}
@@ -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;
@@ -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
@@ -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