Fix memory leak within the @react-facet/dom-fiber (#136)

* reverts package json changes * reverts package json changes again... * Add unit test for unsubscribing from insertBefore/fast-* component. * updates performance benchmark, formats code * removes only test and deferred mount override * updates broken test file --------- Co-authored-by: Marlon Huber-Smith <[email protected]>
Mojang · May 16, 2024 · 9b98b4f · 9b98b4f
1 parent c01086f
commit 9b98b4f
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 9 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -28,7 +28,7 @@ module.exports = {
     'import/no-cycle': 'error',
     'no-unreachable': 'error',
     'no-undef': 'error',
-    'eqeqeq': 'error',
+    eqeqeq: 'error',
     'react-hooks/rules-of-hooks': 'error',
     'react-hooks/exhaustive-deps': 'error',
     'react/jsx-uses-react': 'error',

diff --git a/.github/workflows/benchmarking.yml b/.github/workflows/benchmarking.yml
@@ -36,7 +36,7 @@ jobs:
           node-version-file: '.nvmrc'
           cache: 'yarn'
       - run: yarn install --immutable
-      - run: cd examples/benchmarking && yarn build && yarn compare mountFacetDomFiber mountReactDom 90
+      - run: cd examples/benchmarking && yarn build && yarn compare mountFacetDomFiber mountReactDom 91
 
   overhead:
     runs-on: ubuntu-latest

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -2,7 +2,7 @@
 
 Currently the process of updating the version of the packages is mostly manual. Before you start, first you need to make sure you have the permissions to publish the packages.
 
-- If there is a release branch (see __Release candidate__ below) that hasn't been merged to `main` yet now is the time to do so.
+- If there is a release branch (see **Release candidate** below) that hasn't been merged to `main` yet now is the time to do so.
 - Make sure that you are logged in into your `npm` account. Use the command `yarn npm login` on the project folder to do this.
 - While on the `main` branch.
 - Perform a search an replace on all "package.json" files from the old version to the new. (ex `0.1.4` to `0.2.0`).
@@ -25,4 +25,4 @@ Currently the process of updating the version of the packages is mostly manual.
 - Create an annotated git tag by running `git tag -a v0.4.0-rc.0` (replace with the version). The tag message can be the version again.
 - Push commit and the tag `git push --follow-tags`.
 - Publish the packages by running `yarn publish --tag rc` (it will also build the packages).
-- We are currently not doing release notes for release candidates so you are all done! 🎉
+- We are currently not doing release notes for release candidates so you are all done! 🎉
diff --git a/packages/@react-facet/dom-fiber/src/setupHostConfig.spec.tsx b/packages/@react-facet/dom-fiber/src/setupHostConfig.spec.tsx
@@ -2041,6 +2041,37 @@ describe('umnount', () => {
     expect(unsubscribe).toHaveBeenCalledTimes(1)
   })
 
+  it('unsubscribes from a facet (via a fast-* component) inserted using insertBefore, when the parent is unmounted', () => {
+    const unsubscribe = jest.fn()
+
+    const facet: Facet<string> = {
+      get: () => 'abc',
+      observe: jest.fn().mockReturnValue(unsubscribe),
+    }
+
+    const TestComponent = ({ show, facet }: { facet: Facet<string>; show?: boolean }) => (
+      <div>
+        {show ? <fast-text text={facet} /> : null}
+        <div />
+      </div>
+    )
+
+    render(<TestComponent facet={facet} />)
+
+    expect(facet.observe).toHaveBeenCalledTimes(0)
+    expect(unsubscribe).toHaveBeenCalledTimes(0)
+
+    render(<TestComponent facet={facet} show />)
+
+    expect(facet.observe).toHaveBeenCalledTimes(1)
+    expect(unsubscribe).toHaveBeenCalledTimes(0)
+
+    render(<></>)
+
+    expect(facet.observe).toHaveBeenCalledTimes(1)
+    expect(unsubscribe).toHaveBeenCalledTimes(1)
+  })
+
   it('keeps the subscription of facets when moving in a keyed list', () => {
     const unsubscribeA = jest.fn()
 

diff --git a/tsconfig.json b/tsconfig.json
@@ -1,11 +1,11 @@
 {
   "compilerOptions": {
     "baseUrl": ".",
-		"paths": {
-			"@react-facet/core": ["packages/@react-facet/core/*"],
-			"@react-facet/dom-fiber": ["packages/@react-facet/dom-fiber/*"],
-			"@react-facet/dom-fiber-testing-library": ["packages/@react-facet/dom-fiber-testing-library/*"]
-		},
+    "paths": {
+      "@react-facet/core": ["packages/@react-facet/core/*"],
+      "@react-facet/dom-fiber": ["packages/@react-facet/dom-fiber/*"],
+      "@react-facet/dom-fiber-testing-library": ["packages/@react-facet/dom-fiber-testing-library/*"]
+    },
     "outDir": "dist",
     "target": "es6",
     "moduleResolution": "node",