-
-
Notifications
You must be signed in to change notification settings - Fork 733
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
fix: fix memory leak when using nock with jest #2572
Conversation
Thank you for taking on this problem! Unfortunately your change seems to have broken the Node 10 and 12 tests (I know). Maybe only run |
2d825a0
to
c9548dd
Compare
Hey @gr2m, just updated the job |
c9548dd
to
0e5badd
Compare
Looks like need to fix the command for windows as well 😄 |
0e5badd
to
3fdae85
Compare
@gr2m should be ok now, checked in my repo |
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.
Thank you!
🎉 This PR is included in version 13.5.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
hm... the removal of const module = {
http: require('http'),
https: require('https'),
}[moduleName] broke my tests. Patching it back in fixed it. I didn't look into the root causes but obviously |
@perrin4869 could you create a minimal reproduction repository? |
well of course I'd like to setup a minimal reproduction and write a regression test to make sure it doesn't break again in the future ^^;; |
it's most likely weird lambda code bundling ... I've been bitten by this before. I'd suggest we revert this particular change and add a comment with a warning for future contributors to ... just leave it alone 😁 |
@gr2m that's be super helpful! I'm sure it was coded that way for a reason 😅 |
Thanks for the fix @VladimirChuprazov ! I'm curious whether https://www.npmjs.com/package/nock#memory-issues-with-jest is still relevant with this fix now, would you know? |
@markmssd |
Still required :) |
Just importing nock in a Jest test is enough to have a memory leak.
Fix for #2358