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

use native AbortController #11753

Merged

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Apr 4, 2024

This is a follow-up to #11751

Using the native AbortController previously created a memory leak in our tests, because mock-fetch keeps a history of requests - which referenced the signal - and the signal keeps a reference to the AbortController, so the AbortController itself was never cleared up, and the Request somehow stayed in memory, including a reference to our ref.

Copy link

changeset-bot bot commented Apr 4, 2024

⚠️ No Changeset found

Latest commit: dee80bc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@phryneas phryneas requested a review from alessbell April 4, 2024 13:56
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

Thanks for diving in and solving this mystery 🎉

// that includes the `signal` option, which (with the native `AbortController`)
// has a reference to the `Request` instance, which will somehow reference our
// hash object
fetchMock.resetHistory();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how much overhead this would add, but: thoughts on calling this in an afterEach to make these tests future-proof?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so - in this case, it was even within the test.

In the long run, we should probably move away from mock-fetch, so I wouldn't spend too much time with it now, tbh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right! Forgot that it wasn't just at the top of the test :/ makes sense 👍

@github-actions github-actions bot added the auto-cleanup 🤖 label Apr 4, 2024
@alessbell alessbell merged commit 062394e into custom-jsdom-environment Apr 4, 2024
34 checks passed
@alessbell alessbell deleted the pr/enable-native-abortController branch April 4, 2024 15:55
alessbell added a commit that referenced this pull request Apr 4, 2024
… in the environemt (#11751)

* fix: use custom jsdom env insted of patching it via patch package

* Clean up Prettier, Size-limit, and Api-Extractor

* use native `AbortController` (#11753)

---------

Co-authored-by: Lenz Weber-Tronic <[email protected]>
Co-authored-by: phryneas <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants