Merge pull request #1766 from hypothesis/avoid-ng-lookup-for-services

Decouple `wrapReactComponent` from Angular `$injector`

src/sidebar/index.js

@@ -251,6 +251,8 @@ function startAngularApp(config) {
       .register('$rootScope', { value: $rootScope });
   }
 
+  const wrapComponent = component => wrapReactComponent(component, container);
+
   angular
     .module('h', [angularRoute, angularToastr])
 
@@ -259,36 +261,33 @@ function startAngularApp(config) {
 
     // UI components
     .component('annotation', annotation)
-    .component('annotationBody', wrapReactComponent(AnnotationBody))
-    .component('annotationHeader', wrapReactComponent(AnnotationHeader))
-    .component('annotationActionBar', wrapReactComponent(AnnotationActionBar))
-    .component('annotationLicense', wrapReactComponent(AnnotationLicense))
-    .component('annotationOmega', wrapReactComponent(AnnotationOmega))
+    .component('annotationBody', wrapComponent(AnnotationBody))
+    .component('annotationHeader', wrapComponent(AnnotationHeader))
+    .component('annotationActionBar', wrapComponent(AnnotationActionBar))
+    .component('annotationLicense', wrapComponent(AnnotationLicense))
+    .component('annotationOmega', wrapComponent(AnnotationOmega))
     .component(
       'annotationPublishControl',
-      wrapReactComponent(AnnotationPublishControl)
+      wrapComponent(AnnotationPublishControl)
     )
-    .component('annotationQuote', wrapReactComponent(AnnotationQuote))
+    .component('annotationQuote', wrapComponent(AnnotationQuote))
     .component('annotationThread', annotationThread)
     .component('annotationViewerContent', annotationViewerContent)
-    .component('helpPanel', wrapReactComponent(HelpPanel))
-    .component('loggedOutMessage', wrapReactComponent(LoggedOutMessage))
-    .component('moderationBanner', wrapReactComponent(ModerationBanner))
-    .component('searchStatusBar', wrapReactComponent(SearchStatusBar))
-    .component('focusedModeHeader', wrapReactComponent(FocusedModeHeader))
-    .component('selectionTabs', wrapReactComponent(SelectionTabs))
+    .component('helpPanel', wrapComponent(HelpPanel))
+    .component('loggedOutMessage', wrapComponent(LoggedOutMessage))
+    .component('moderationBanner', wrapComponent(ModerationBanner))
+    .component('searchStatusBar', wrapComponent(SearchStatusBar))
+    .component('focusedModeHeader', wrapComponent(FocusedModeHeader))
+    .component('selectionTabs', wrapComponent(SelectionTabs))
     .component('sidebarContent', sidebarContent)
-    .component('sidebarContentError', wrapReactComponent(SidebarContentError))
-    .component(
-      'shareAnnotationsPanel',
-      wrapReactComponent(ShareAnnotationsPanel)
-    )
+    .component('sidebarContentError', wrapComponent(SidebarContentError))
+    .component('shareAnnotationsPanel', wrapComponent(ShareAnnotationsPanel))
     .component('streamContent', streamContent)
-    .component('svgIcon', wrapReactComponent(SvgIcon))
-    .component('tagEditor', wrapReactComponent(TagEditor))
-    .component('tagList', wrapReactComponent(TagList))
+    .component('svgIcon', wrapComponent(SvgIcon))
+    .component('tagEditor', wrapComponent(TagEditor))
+    .component('tagList', wrapComponent(TagList))
     .component('threadList', threadList)
-    .component('topBar', wrapReactComponent(TopBar))
+    .component('topBar', wrapComponent(TopBar))
     .directive('hAutofocus', hAutofocusDirective)
     .directive('hBranding', hBrandingDirective)
     .directive('hOnTouch', hOnTouchDirective)
@@ -315,7 +314,6 @@ function startAngularApp(config) {
     .service('session', () => container.get('session'))
     .service('streamer', () => container.get('streamer'))
     .service('streamFilter', () => container.get('streamFilter'))
-    .service('tags', () => container.get('tags'))
 
     // Redux store
     .service('store', () => container.get('store'))

src/sidebar/util/service-context.js

@@ -64,7 +64,7 @@ export function withServices(Component) {
   function Wrapper(props) {
     // Get the current dependency injector instance that is provided by a
     // `ServiceContext.Provider` somewhere higher up the component tree.
-    const $injector = useContext(ServiceContext);
+    const injector = useContext(ServiceContext);
 
     // Inject services, unless they have been overridden by props passed from
     // the parent component.
@@ -80,7 +80,7 @@ export function withServices(Component) {
       }
 
       if (!(service in props)) {
-        services[service] = $injector.get(service);
+        services[service] = injector.get(service);
       }
     }
     return <Component {...services} {...props} />;

src/sidebar/util/test/wrap-react-component-test.js

@@ -3,6 +3,7 @@ import { Component, createElement } from 'preact';
 import { useContext } from 'preact/hooks';
 import propTypes from 'prop-types';
 
+import { Injector } from '../../../shared/injector';
 import { createDirective } from '../../directive/test/util';
 import { ServiceContext } from '../service-context';
 import wrapReactComponent from '../wrap-react-component';
@@ -58,11 +59,15 @@ describe('wrapReactComponent', () => {
     return { element, onClick, onDblClick };
   }
 
+  let servicesInjector;
+
   beforeEach(() => {
+    const servicesInjector = new Injector();
+    servicesInjector.register('theme', { value: 'dark' });
+
     angular
       .module('app', [])
-      .component('btn', wrapReactComponent(Button))
-      .value('theme', 'dark');
+      .component('btn', wrapReactComponent(Button, servicesInjector));
     angular.mock.module('app');
   });
 
@@ -73,7 +78,7 @@ describe('wrapReactComponent', () => {
   });
 
   it('derives Angular component "bindings" from React "propTypes"', () => {
-    const ngComponent = wrapReactComponent(Button);
+    const ngComponent = wrapReactComponent(Button, servicesInjector);
     assert.deepEqual(ngComponent.bindings, {
       label: '<',
       isDisabled: '<',
@@ -141,7 +146,7 @@ describe('wrapReactComponent', () => {
     angular
       .module('app', [])
       .component('parent', parentComponent)
-      .component('child', wrapReactComponent(ChildComponent));
+      .component('child', wrapReactComponent(ChildComponent, servicesInjector));
     angular.mock.module('app');
 
     // Render the component with the child initially visible.
@@ -160,7 +165,7 @@ describe('wrapReactComponent', () => {
       return <div>Hello world</div>;
     }
     assert.throws(
-      () => wrapReactComponent(TestComponent),
+      () => wrapReactComponent(TestComponent, servicesInjector),
       'React component TestComponent does not specify its inputs using "propTypes"'
     );
   });
@@ -231,7 +236,7 @@ describe('wrapReactComponent', () => {
     angular
       .module('app', [])
       .component('parent', parentComponent)
-      .component('child', wrapReactComponent(Child));
+      .component('child', wrapReactComponent(Child, servicesInjector));
     angular.mock.module('app');
 
     const element = createDirective(document, 'parent');

src/sidebar/util/wrap-react-component.js

@@ -6,6 +6,10 @@ function useExpressionBinding(propName) {
   return propName.match(/^on[A-Z]/);
 }
 
+/**
+ * @typedef {import('../../shared/injector').Injector} Injector
+ */
+
 /**
  * Controller for an Angular component that wraps a React component.
  *
@@ -14,14 +18,16 @@ function useExpressionBinding(propName) {
  * has been created.
  */
 class ReactController {
-  constructor($element, $injector, $scope, type) {
+  constructor($element, services, $scope, type) {
     /** The DOM element where the React component should be rendered. */
     this.domElement = $element[0];
 
     /**
-     * The Angular service injector, used by this component and its descendants.
+     * The services injector, used by this component and its descendants.
+     *
+     * @type {Injector}
      */
-    this.$injector = $injector;
+    this.services = services;
 
     /** The React component function or class. */
     this.type = type;
@@ -83,10 +89,10 @@ class ReactController {
 
   updateReactComponent() {
     // Render component, with a `ServiceContext.Provider` wrapper which
-    // provides access to Angular services via `withServices` or `useContext`
+    // provides access to services via `withServices` or `useContext`
     // in child components.
     render(
-      <ServiceContext.Provider value={this.$injector}>
+      <ServiceContext.Provider value={this.services}>
         <this.type {...this.props} />
       </ServiceContext.Provider>,
       this.domElement
@@ -103,14 +109,16 @@ class ReactController {
  * one-way ('<') bindings except for those with names matching /^on[A-Z]/ which
  * are assumed to be callbacks that use expression ('&') bindings.
  *
- * If the React component needs access to an Angular service, it can get at
+ * If the React component needs access to a service, it can get at
  * them using the `withServices` wrapper from service-context.js.
  *
  * @param {Function} type - The React component class or function
+ * @param {Injector} services
+ *   Dependency injection container providing services that components use.
  * @return {Object} -
  *   An AngularJS component spec for use with `angular.component(...)`
  */
-export default function wrapReactComponent(type) {
+export default function wrapReactComponent(type, services) {
   if (!type.propTypes) {
     throw new Error(
       `React component ${type.name} does not specify its inputs using "propTypes"`
@@ -122,8 +130,8 @@ export default function wrapReactComponent(type) {
    * component being wrapped.
    */
   // @ngInject
-  function createController($element, $injector, $scope) {
-    return new ReactController($element, $injector, $scope, type);
+  function createController($element, $scope) {
+    return new ReactController($element, services, $scope, type);
   }
 
   const bindings = {};