Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix provider global clenup on error #89

Merged
merged 3 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/eslint/rules/no-extraneous.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
},

Expand Down
48 changes: 47 additions & 1 deletion src/react/__tests__/provider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an interesting test, look like there is a story behind it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because context provider memo can be fiddly 😅

});

describe('getDependencies()', () => {
const Text = () => 'text';
const Button = forwardRef(() => 'button');
Expand Down Expand Up @@ -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(() => {
Expand Down
66 changes: 42 additions & 24 deletions src/react/provider.js
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check - :notsureif: unmount not happens during catch.
I have a very string dejavu feeling right now, like we had this conversation already...

I just cannot imagine that some allocated resources (any) can be "not freed" in case of emergency. That would like a disaster!

Copy link
Owner Author

@albertogasparin albertogasparin Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not, just tested: neither useEffects cleanup nor componentWillUnmount will fire if a child throws during render (just checked). 😢

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something we need to clarify for ourselves

}

componentWillUnmount() {
globalDi._remove(this.props.use);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are not the same use you've got during the initial render and later used for _fromProvider, these are different.

There were a few bugs due to mismatches of this sort, I would better remeber a "cleanup callback" somewhere in getValue

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come? I would expect them to be consistent 🤔 Unless someone decides to conditionally add/remove injectables (which we call out as not supported) it should contain the same "stuff"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, stuff happens.
This is not a real problem, but a potential.

Ideally just expose another (secret) method to check if global DI is clear. Then we can call it before test to verify that everything is as it should be. Later we can remove this method.

Well, it should be fine, but we all know that our assumptions are not always true

}

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);
Expand All @@ -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);
Expand All @@ -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) => (
Expand Down