Skip to content

Commit

Permalink
Fix memory leak within the @react-facet/dom-fiber (#136)
Browse files Browse the repository at this point in the history
* 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 <marlonhubersmith@gmail.com>
  • Loading branch information
ja-ni and marlonicus committed May 16, 2024
1 parent c01086f commit 9b98b4f
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 9 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/benchmarking.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`).
Expand All @@ -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! 馃帀
31 changes: 31 additions & 0 deletions packages/@react-facet/dom-fiber/src/setupHostConfig.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
10 changes: 5 additions & 5 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down

0 comments on commit 9b98b4f

Please sign in to comment.