-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed memory leak in rapid hook arg changing #4268
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 527a6b7:
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts
Outdated
Show resolved
Hide resolved
fireEvent.click(screen.getByTestId('fetchButton')) | ||
|
||
await act(async () => { | ||
await delay(1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As that hasn't been defined in this file yet, you'll have to define it somewhere at the top of the file
function delay(ms: number) {
return new Promise((resolve) => setTimeout(resolve, ms))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you should also just be able to re-use the delay
function from msw
:
import { delay } from 'msw'
await delay(1000)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My confusion here stems from the fact that it IS imported here and the test block directly below it uses it without problem.
Most likely I have just missed something painstakingly obvious, but I added a definition in the test block regardless.
Is this going to be merged someday? 😭 |
@santirodriguezaffonso : the PR has been up for only 3 days, and we're all busy atm. Please don't beg for things to be merged. |
and in the meantime, you can click on the link in the codesandbox message to use the build that's already published. |
packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts
Outdated
Show resolved
Hide resolved
Mark already said this, but please be a bit more patient. We are all doing this in our free time, and some things can take days, weeks or even months - it all depends on how many resources we have at any given time, given our day jobs and private lifes we have to balance with voluntary Open Source. |
@santirodriguezaffonso |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No "poor code quality" here - libraries just need different code than applications.
Your code was good from the start, we just have special requirements to keep bundle size down as a library :)
Thank you for the contribution!
) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@reduxjs/toolkit](https://redux-toolkit.js.org) ([source](https://github.com/reduxjs/redux-toolkit)) | [`2.2.1` -> `2.2.2`](https://renovatebot.com/diffs/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/redux-toolkit (@​reduxjs/toolkit)</summary> ### [`v2.2.2`](https://github.com/reduxjs/redux-toolkit/releases/tag/v2.2.2) [Compare Source](https://github.com/reduxjs/redux-toolkit/compare/v2.2.1...v2.2.2) This patch release fixes an incorrect build setting for the `legacy-esm` artifacts, and fixes an issue with RTKQ query hooks didn't always remove the cache entries if arguments were changed rapidly. #### Changes ##### `legacy-esm` Artifact Transpilation The `legacy-esm` build artifacts are intended for use by Webpack 4. Those were *supposed* to be transpiled to target `"es2017"`, but were in fact still set to target `"esnext"` - an oversight during the 2.0 development cycle. This release fixes that setting, so those artifacts are now correctly transpiled. ##### Other Fixes RTKQ query hooks now handle additional actions around argument changes that should result in cache entries being removed. Additionally, 2.2.1 contained a fix to an incorrectly named type: `TypedUseMutationTrigger` is now `TypedMutationTrigger`. #### What's Changed - rename TypedUseMutationTrigger to TypedMutationTrigger, and add deprecated alias by [@​EskiMojo14](https://github.com/EskiMojo14) in [https://github.com/reduxjs/redux-toolkit/pull/4204](https://github.com/reduxjs/redux-toolkit/pull/4204) - Fixed memory leak in rapid hook arg changing by [@​riqts](https://github.com/riqts) in [https://github.com/reduxjs/redux-toolkit/pull/4268](https://github.com/reduxjs/redux-toolkit/pull/4268) - Fix incorrect legacy-esm target by [@​markerikson](https://github.com/markerikson) in [https://github.com/reduxjs/redux-toolkit/pull/4284](https://github.com/reduxjs/redux-toolkit/pull/4284) **Full Changelog**: reduxjs/redux-toolkit@v2.2.0...v2.2.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am on Monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/coveo/ui-kit). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@reduxjs/toolkit](https://redux-toolkit.js.org) ([source](https://github.com/reduxjs/redux-toolkit)) | [`^2.2.1` -> `^2.2.2`](https://renovatebot.com/diffs/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/redux-toolkit (@​reduxjs/toolkit)</summary> ### [`v2.2.2`](https://github.com/reduxjs/redux-toolkit/releases/tag/v2.2.2) [Compare Source](https://github.com/reduxjs/redux-toolkit/compare/v2.2.1...v2.2.2) This patch release fixes an incorrect build setting for the `legacy-esm` artifacts, and fixes an issue with RTKQ query hooks didn't always remove the cache entries if arguments were changed rapidly. #### Changes ##### `legacy-esm` Artifact Transpilation The `legacy-esm` build artifacts are intended for use by Webpack 4. Those were *supposed* to be transpiled to target `"es2017"`, but were in fact still set to target `"esnext"` - an oversight during the 2.0 development cycle. This release fixes that setting, so those artifacts are now correctly transpiled. ##### Other Fixes RTKQ query hooks now handle additional actions around argument changes that should result in cache entries being removed. Additionally, 2.2.1 contained a fix to an incorrectly named type: `TypedUseMutationTrigger` is now `TypedMutationTrigger`. #### What's Changed - rename TypedUseMutationTrigger to TypedMutationTrigger, and add deprecated alias by [@​EskiMojo14](https://github.com/EskiMojo14) in [https://github.com/reduxjs/redux-toolkit/pull/4204](https://github.com/reduxjs/redux-toolkit/pull/4204) - Fixed memory leak in rapid hook arg changing by [@​riqts](https://github.com/riqts) in [https://github.com/reduxjs/redux-toolkit/pull/4268](https://github.com/reduxjs/redux-toolkit/pull/4268) - Fix incorrect legacy-esm target by [@​markerikson](https://github.com/markerikson) in [https://github.com/reduxjs/redux-toolkit/pull/4284](https://github.com/reduxjs/redux-toolkit/pull/4284) **Full Changelog**: reduxjs/redux-toolkit@v2.2.0...v2.2.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Thanks guys! You rock 🤟🏻 |
) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@reduxjs/toolkit](https://redux-toolkit.js.org) ([source](https://github.com/reduxjs/redux-toolkit)) | [`^2.2.1` -> `^2.2.2`](https://renovatebot.com/diffs/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@reduxjs%2ftoolkit/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@reduxjs%2ftoolkit/2.2.1/2.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/redux-toolkit (@​reduxjs/toolkit)</summary> ### [`v2.2.2`](https://github.com/reduxjs/redux-toolkit/releases/tag/v2.2.2) [Compare Source](https://github.com/reduxjs/redux-toolkit/compare/v2.2.1...v2.2.2) This patch release fixes an incorrect build setting for the `legacy-esm` artifacts, and fixes an issue with RTKQ query hooks didn't always remove the cache entries if arguments were changed rapidly. #### Changes ##### `legacy-esm` Artifact Transpilation The `legacy-esm` build artifacts are intended for use by Webpack 4. Those were *supposed* to be transpiled to target `"es2017"`, but were in fact still set to target `"esnext"` - an oversight during the 2.0 development cycle. This release fixes that setting, so those artifacts are now correctly transpiled. ##### Other Fixes RTKQ query hooks now handle additional actions around argument changes that should result in cache entries being removed. Additionally, 2.2.1 contained a fix to an incorrectly named type: `TypedUseMutationTrigger` is now `TypedMutationTrigger`. #### What's Changed - rename TypedUseMutationTrigger to TypedMutationTrigger, and add deprecated alias by [@​EskiMojo14](https://github.com/EskiMojo14) in [https://github.com/reduxjs/redux-toolkit/pull/4204](https://github.com/reduxjs/redux-toolkit/pull/4204) - Fixed memory leak in rapid hook arg changing by [@​riqts](https://github.com/riqts) in [https://github.com/reduxjs/redux-toolkit/pull/4268](https://github.com/reduxjs/redux-toolkit/pull/4268) - Fix incorrect legacy-esm target by [@​markerikson](https://github.com/markerikson) in [https://github.com/reduxjs/redux-toolkit/pull/4284](https://github.com/reduxjs/redux-toolkit/pull/4284) **Full Changelog**: reduxjs/redux-toolkit@v2.2.0...v2.2.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Fixes #4267
Tests failed in github actions referencing delay not being defined but I am not sure what to do to fix that, I did not add the import for delay, simply referenced it. Any direction for that would be appreciated.