Fix provider global clenup on error (#89)

albertogasparin · web-flow · commit 2c2adf7dbfb1 · 2024-01-05T15:57:25.000+11:00
diff --git a/examples/.babelrc.js b/examples/.babelrc.js
@@ -3,5 +3,8 @@
 module.exports = {
   extends: '../babel.config.js',
   presets: ['@babel/preset-env', '@babel/preset-react', '@babel/preset-flow'],
-  plugins: ['@babel/plugin-proposal-class-properties', '../src/babel'],
+  plugins: [
+    '@babel/plugin-proposal-class-properties',
+    ['../src/babel', { enabledEnvs: ['development', 'test', undefined] }],
+  ],
 };
diff --git a/examples/flow/components/input.js b/examples/flow/components/input.js
@@ -1,6 +1,5 @@
 // @flow
 import React, { useState, type Node } from 'react';
-import { di } from 'react-magnetic-di';
 
 type UseThemeState = { color: string };
 export function useTheme(): $Call<typeof useState, UseThemeState> {
@@ -12,7 +11,6 @@ export type InputProps = {|
 |};
 
 export function Input(props: InputProps): Node {
-  di(useTheme);
   const [style] = useTheme();
 
   return (
diff --git a/examples/flow/components/label.js b/examples/flow/components/label.js
@@ -1,6 +1,5 @@
 // @flow
 import React, { useState, type Node } from 'react';
-import { di } from 'react-magnetic-di';
 
 type UseThemeState = { color: string };
 export function useTheme(): $Call<typeof useState, UseThemeState> {
@@ -12,7 +11,6 @@ type LabelProps = {|
 |};
 
 export function Label({ children = 'Label' }: LabelProps): Node {
-  di(useTheme);
   const [style] = useTheme();
   return <div style={style}>{children}</div>;
 }
diff --git a/examples/flow/components/section.js b/examples/flow/components/section.js
@@ -1,6 +1,5 @@
 // @flow
 import React, { Component, type Node } from 'react';
-import { di } from 'react-magnetic-di';
 
 import { Label } from './label';
 import { Input } from './input';
@@ -11,8 +10,6 @@ type Props = {|
 
 export class Section extends Component<Props> {
   render(): Node {
-    di(Input, Label);
-
     const { title } = this.props;
     return (
       <div>
diff --git a/examples/global/index.ts b/examples/global/index.ts
@@ -1,17 +1,13 @@
-import { di } from 'react-magnetic-di';
-
 type Data = { data: number[] };
 
 export const fetchApi = async (): Promise<Data> => ({ data: [1, 2] });
 export const processApiData = (data: Data['data']) => data.map((v) => v * v);
 
 export function transformer(response: Data) {
-  di(processApiData);
   return processApiData(response.data);
 }
 
 export async function apiHandler() {
-  di(fetchApi, transformer);
   const data = await fetchApi();
   return transformer(data);
 }
diff --git a/examples/typescript/components/input.tsx b/examples/typescript/components/input.tsx
@@ -1,6 +1,5 @@
 // @flow
 import React, { useState } from 'react';
-import { di } from 'react-magnetic-di';
 
 type UseThemeState = { color: string };
 export function useTheme() {
@@ -12,7 +11,6 @@ export type InputProps = {
 };
 
 export function Input(props: InputProps) {
-  di(useTheme);
   const [style] = useTheme();
 
   return (
diff --git a/examples/typescript/components/label.tsx b/examples/typescript/components/label.tsx
@@ -1,6 +1,4 @@
-// @flow
 import React, { type ReactNode, useState } from 'react';
-import { di } from 'react-magnetic-di';
 
 type UseThemeState = { color: string };
 export function useTheme() {
@@ -12,7 +10,6 @@ type LabelProps = {
 };
 
 export function Label({ children = 'Label' }: LabelProps) {
-  di(useTheme);
   const [style] = useTheme();
   return <div style={style}>{children}</div>;
 }
diff --git a/examples/typescript/components/section.tsx b/examples/typescript/components/section.tsx
@@ -1,6 +1,4 @@
-// @flow
 import React, { Component } from 'react';
-import { di } from 'react-magnetic-di';
 
 import { Label } from './label';
 import { Input } from './input';
@@ -11,8 +9,6 @@ type Props = {
 
 export class Section extends Component<Props> {
   render() {
-    di(Input, Label);
-
     const { title } = this.props;
     return (
       <div>
diff --git a/src/eslint/rules/no-extraneous.js b/src/eslint/rules/no-extraneous.js
@@ -62,7 +62,9 @@ module.exports = {
 
         blockReferences.set(node, {
           di: diVars,
-          through: context.getScope().through.map((v) => v.identifier),
+          through: context.sourceCode
+            .getScope(node)
+            .through.map((v) => v.identifier),
         });
       },
 
diff --git a/src/index.js b/src/index.js
@@ -1,5 +1,5 @@
 export { di } from './react/consumer';
-export { runWithDi } from './react/global';
+export { runWithDi, globalDi as unstable_globalDi } from './react/global';
 export { DiProvider, withDi } from './react/provider';
 export { injectable } from './react/injectable';
 export { debug } from './react/utils';
diff --git a/src/react/__tests__/provider.test.js b/src/react/__tests__/provider.test.js
@@ -3,7 +3,7 @@
  */
 /* eslint-env jest */
 
-import React, { forwardRef } from 'react';
+import React, { Component, forwardRef } from 'react';
 import { render } from '@testing-library/react';
 
 import { Context } from '../context';
@@ -27,6 +27,20 @@ describe('DiProvider', () => {
     });
   });
 
+  it('should expose stable context', () => {
+    const children = jest.fn();
+    const App = () => (
+      <DiProvider use={[]}>
+        <Context.Consumer>{children}</Context.Consumer>
+      </DiProvider>
+    );
+
+    const { rerender } = render(<App />);
+    rerender(<App />);
+
+    expect(children.mock.calls[0][0]).toBe(children.mock.calls[1][0]);
+  });
+
   describe('getDependencies()', () => {
     const Text = () => 'text';
     const Button = forwardRef(() => 'button');
@@ -161,6 +175,38 @@ describe('DiProvider', () => {
     expect(globalDi._remove).toHaveBeenCalledWith([TextDi2]);
   });
 
+  it('should remove global injection on error', () => {
+    jest.spyOn(globalDi, '_remove');
+    const cSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+
+    const children = jest.fn().mockImplementation(() => {
+      throw new Error('Oops!');
+    });
+    const TextDi = injectable(Text, () => '', { global: true });
+    const TextDi2 = injectable(Text, () => '');
+    class ErrorBoundary extends Component {
+      componentDidCatch() {
+        cSpy.mockRestore();
+      }
+      render() {
+        return this.props.children;
+      }
+    }
+
+    render(
+      <ErrorBoundary>
+        <DiProvider use={[TextDi]}>
+          <DiProvider use={[TextDi2]} global>
+            <Context.Consumer>{children}</Context.Consumer>
+          </DiProvider>
+        </DiProvider>
+      </ErrorBoundary>
+    );
+
+    expect(globalDi._remove).toHaveBeenCalledWith([TextDi]);
+    expect(globalDi._remove).toHaveBeenCalledWith([TextDi2]);
+  });
+
   it('should error when a non injectable is used', () => {
     const cSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
     expect(() => {
diff --git a/src/react/provider.js b/src/react/provider.js
@@ -1,16 +1,42 @@
-import React, { useContext, useMemo, forwardRef, useEffect } from 'react';
+import React, { forwardRef, Component } from 'react';
 import PropTypes from 'prop-types';
 
 import { diRegistry } from './constants';
 import { Context } from './context';
 import { addInjectableToMap, getDisplayName, findInjectable } from './utils';
 import { globalDi } from './global';
 
-export const DiProvider = ({ children, use, target, global }) => {
-  const { getDependencies } = useContext(Context);
+export class DiProvider extends Component {
+  static contextType = Context;
+
+  static propTypes = {
+    children: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),
+    global: PropTypes.bool,
+    target: PropTypes.oneOfType([
+      PropTypes.func,
+      PropTypes.arrayOf(PropTypes.func),
+    ]),
+    use: PropTypes.arrayOf(
+      PropTypes.oneOfType([PropTypes.func, PropTypes.object])
+    ).isRequired,
+  };
+
+  componentDidCatch(err) {
+    globalDi._remove(this.props.use);
+    throw err;
+  }
+
+  componentWillUnmount() {
+    globalDi._remove(this.props.use);
+  }
+
+  value = undefined;
+  getValue() {
+    if (this.value) return this.value;
+
+    const { use, target, global } = this.props;
+    const { getDependencies } = this.context;
 
-  // memo provider value so gets computed only once
-  const value = useMemo(() => {
     // create a map of dependency real -> replacements for fast lookup
     const replacementMap = use.reduce((acc, inj) => {
       addInjectableToMap(acc, inj);
@@ -21,7 +47,7 @@ export const DiProvider = ({ children, use, target, global }) => {
     // support single or multiple targets
     const targets = target && (Array.isArray(target) ? target : [target]);
 
-    return {
+    this.value = {
       getDependencies(realDeps, targetChild) {
         // First we collect dependencies from parent provider(s) (if any)
         const dependencies = getDependencies(realDeps, targetChild);
@@ -44,25 +70,17 @@ export const DiProvider = ({ children, use, target, global }) => {
         return dependencies;
       },
     };
-  }, [getDependencies]); // ignore use & target props
-
-  // on unmount
-  useEffect(() => () => globalDi._remove(use), []); // ignore use prop
+    return this.value;
+  }
 
-  return <Context.Provider value={value}>{children}</Context.Provider>;
-};
-
-DiProvider.propTypes = {
-  children: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),
-  global: PropTypes.bool,
-  target: PropTypes.oneOfType([
-    PropTypes.func,
-    PropTypes.arrayOf(PropTypes.func),
-  ]),
-  use: PropTypes.arrayOf(
-    PropTypes.oneOfType([PropTypes.func, PropTypes.object])
-  ).isRequired,
-};
+  render() {
+    return (
+      <Context.Provider value={this.getValue()}>
+        {this.props.children}
+      </Context.Provider>
+    );
+  }
+}
 
 export function withDi(Comp, deps, target = null) {
   const WrappedComponent = forwardRef((props, ref) => (
