From d551bf5ca4f79280d6fd123614f4040a55bc3f1a Mon Sep 17 00:00:00 2001 From: Guy Carmeli Date: Tue, 18 Jun 2024 20:58:58 +0300 Subject: [PATCH] Fix leaking graphs when classes are injected by lifecycle bound graphs When a React construct component is injected by a lifecycle bound graph, the graph is retained in memory until the React construct is unmounted. Injecting regular classes also contributed to the retention count which caused a bug as graphs are released on unmount. --- .../src/graph/registry/GraphRegistry.ts | 4 +- .../src/injectors/class/ClassInjector.ts | 8 +++- .../integration/lifecyleBoundGraphs.test.tsx | 45 +++++++++++++++++-- 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/packages/react-obsidian/src/graph/registry/GraphRegistry.ts b/packages/react-obsidian/src/graph/registry/GraphRegistry.ts index 2656bb9c..11a7ce1d 100644 --- a/packages/react-obsidian/src/graph/registry/GraphRegistry.ts +++ b/packages/react-obsidian/src/graph/registry/GraphRegistry.ts @@ -32,13 +32,13 @@ export class GraphRegistry { resolve( Graph: Constructable, - source: 'lifecycleOwner' | 'serviceLocator' = 'lifecycleOwner', + source: 'lifecycleOwner' | 'classInjection' | 'serviceLocator' = 'lifecycleOwner', props: any = undefined, ): T { if ((this.isSingleton(Graph) || this.isBoundToReactLifecycle(Graph)) && this.has(Graph)) { return this.getFirst(Graph); } - if (this.isBoundToReactLifecycle(Graph) && source === 'serviceLocator') { + if (this.isBoundToReactLifecycle(Graph) && source !== 'lifecycleOwner') { throw new ObtainLifecycleBoundGraphException(Graph); } const graph = this.graphMiddlewares.resolve(Graph, props); diff --git a/packages/react-obsidian/src/injectors/class/ClassInjector.ts b/packages/react-obsidian/src/injectors/class/ClassInjector.ts index 1d0608f2..367859f5 100644 --- a/packages/react-obsidian/src/injectors/class/ClassInjector.ts +++ b/packages/react-obsidian/src/injectors/class/ClassInjector.ts @@ -24,8 +24,12 @@ export default class ClassInjector { ): ProxyHandler { return new class Handler implements ProxyHandler { construct(target: any, args: any[], newTarget: Function): any { - const graph = graphRegistry.resolve(Graph, 'lifecycleOwner', args.length > 0 ? args[0] : undefined); - referenceCounter.retain(graph); + const isReactClassComponent = target.prototype?.isReactComponent; + const source = isReactClassComponent ? 'lifecycleOwner' : 'classInjection'; + const graph = graphRegistry.resolve(Graph, source, args.length > 0 ? args[0] : undefined); + if (isReactClassComponent) { + referenceCounter.retain(graph); + } Reflect.defineMetadata(GRAPH_INSTANCE_NAME_KEY, graph.name, target); const argsToInject = this.injectConstructorArgs(args, graph, target); graph.onBind(target); diff --git a/packages/react-obsidian/test/integration/lifecyleBoundGraphs.test.tsx b/packages/react-obsidian/test/integration/lifecyleBoundGraphs.test.tsx index cd8ccd75..3aeda36d 100644 --- a/packages/react-obsidian/test/integration/lifecyleBoundGraphs.test.tsx +++ b/packages/react-obsidian/test/integration/lifecyleBoundGraphs.test.tsx @@ -8,6 +8,7 @@ import { injectHook, } from '../../src'; import { LifecycleBoundGraph } from '../fixtures/LifecycleBoundGraph'; +import { ObtainLifecycleBoundGraphException } from '../../src/graph/registry/ObtainLifecycleBoundGraphException'; describe('React lifecycle bound graphs', () => { const Component = createFunctionalComponent(); @@ -30,18 +31,39 @@ describe('React lifecycle bound graphs', () => { expect(LifecycleBoundGraph.timesCreated).toBe(2); }); + it('clears a bound graph after dependent components are unmounted when it was used for class injection', () => { + const Component2 = createFunctionalComponent({ instantiateInjectableClass: true}); + const { unmount } = render(); + unmount(); + render(); + + expect(LifecycleBoundGraph.timesCreated).toBe(2); + }); + it('passes props to the component', () => { const { container } = render(); expect(container.textContent).toBe('A string passed via props: Obsidian is cool'); }); it('obtains a lifecycle bound graph only if it was already created', () => { - expect(() => Obsidian.obtain(LifecycleBoundGraph)).toThrowError( + expect(() => Obsidian.obtain(LifecycleBoundGraph)).toThrow( 'Tried to obtain a @LifecycleBound graph LifecycleBoundGraph, but it was not created yet. ' + '@LifecycleBound graphs can only be obtained after they were created by a React component or hook.', ); }); + it('throws when a lifecycle bound graph is used to inject a class before it was created', () => { + expect(() => { + @Injectable(LifecycleBoundGraph) + class Foo { + // @ts-ignore + @Inject() private computedFromProps!: string; + } + // eslint-disable-next-line no-new + new Foo(); + }).toThrow(ObtainLifecycleBoundGraphException); + }); + it(`resolves a dependency when @LifecycleBound graph is used as a service locator`, () => { render(); @@ -64,8 +86,16 @@ describe('React lifecycle bound graphs', () => { expect(LifecycleBoundGraph.timesCreated).toBe(2); }); - function createFunctionalComponent() { - const useHook = injectHook(() => {}, LifecycleBoundGraph); + type CreateOptions = {instantiateInjectableClass: boolean}; + function createFunctionalComponent({instantiateInjectableClass}: CreateOptions = { + instantiateInjectableClass: false, + }) { + const useHook = injectHook(() => { + if (instantiateInjectableClass) { + // eslint-disable-next-line no-new + new Foo(); + } + }, LifecycleBoundGraph); return injectComponent(() => { useHook(); @@ -81,4 +111,13 @@ describe('React lifecycle bound graphs', () => { return <>{this.computedFromProps}; } } + + @Injectable(LifecycleBoundGraph) + class Foo { + @Inject() private computedFromProps!: string; + + log() { + console.log(this.computedFromProps); + } + } });