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

Doesn't work with weak on Node 12 #8397

Closed
kirillgroshkov opened this issue Apr 30, 2019 · 22 comments
Closed

Doesn't work with weak on Node 12 #8397

kirillgroshkov opened this issue Apr 30, 2019 · 22 comments

Comments

@kirillgroshkov
Copy link

kirillgroshkov commented Apr 30, 2019

Jest has --detectLeaks amazing feature that is powered by weak npm package.

The problem is that weak doesn't work with Node 12 (I've opened an issue there: TooTallNate/node-weak#99).

The best way would be for them to fix it. But, it looks not very maintained ATM.

At the same time I found that @danfuzz there suggesting to use other package as API-compatible replacement for weak that is using a sustainable NAPI: https://github.com/node-ffi-napi/weak-napi

Should we consider to try it and if it works to switch Jest to use that package instead?

@SimenB
Copy link
Member

SimenB commented Apr 30, 2019

Very much open to switching!

@SimenB
Copy link
Member

SimenB commented Apr 30, 2019

FWIW I flagged this here: nodejs/node#25060 (comment)

@danfuzz
Copy link

danfuzz commented Apr 30, 2019

👋 FWIW I continue to have a good experience with weak-napi.

@SimenB
Copy link
Member

SimenB commented May 2, 2019

Wanted to add node 12 to our test matrix, and it fails due to being unable to install weak. Wanna send a PR switching to the linked package? 😀 If it doesn't support node 6, we'll be dropping it as soon as we get the next release out (today, hopefully)

@lh0x00
Copy link
Contributor

lh0x00 commented May 4, 2019

@SimenB Hi, I have a question about we can require the package based Node version?

@SimenB
Copy link
Member

SimenB commented May 4, 2019

sure

@lh0x00
Copy link
Contributor

lh0x00 commented May 4, 2019

Thank you @SimenB , I waiting to try Jest run in the new Node version.

@SimenB
Copy link
Member

SimenB commented Jul 4, 2019

I tried this here: https://github.com/SimenB/jest/tree/weak-napi

But the tests fail... Not sure if it's a bug in the napi implementation or not

@lh0x00
Copy link
Contributor

lh0x00 commented Jul 13, 2019

@SimenB I created a #8686 to resolve this problem, can you review it?

@niieani
Copy link
Contributor

niieani commented Aug 6, 2019

It's also possible to solve this by replacing weak with one of the forks in the upstream PR:

    "weak": "github:lxe/node-weak#node-12"

@SimenB
Copy link
Member

SimenB commented Aug 6, 2019

That can probably be done using the resolutions field if using yarn as well

@SimenB
Copy link
Member

SimenB commented Aug 19, 2019

Solved by #8686, which will be available in Jest 25 (at some point, hope to get an alpha out soonish, though)

@SimenB SimenB closed this as completed Aug 19, 2019
@MischkowskyM
Copy link

The choice weak-napi is a bit unfortunate. I can't install jest@next / jest 25 in the office now, due to it's transient dependency on windows-build-tools and license implications that has, as well as the required admin privilege to install possibly 'unsanctioned' software globally on my machine (and our build agents).

I could probably get the permission(s) required, but I'd rather not go through that process.

Is there an alternative to weak-napi?

@SimenB
Copy link
Member

SimenB commented Aug 22, 2019

I opened up a PR 2 minutes ago which makes it optional. 😅 #8869. It means you won't be able to use --detect-leaks unless you get those permissions. However, install shouldn't fail.

Is there an alternative to weak-napi?

AFAIK, not really - at least not in a way that'll allow you to use it without compilation - it's a C++ API so we have to build a native addon for it

@kirill-konshin
Copy link

kirill-konshin commented Oct 2, 2019

In which version of Jest is it available after all? Latest release was on Aug 15, so looks like this fix has not been released yet?

@kirill-konshin
Copy link

Are there any news? I still don't see any releases since August.

@dylanwulf
Copy link

I'm trying [email protected] available from npm, and it seems to work with node v12 but when I use the --detectLeaks option, every single test fails with a detected leak (even if the only thing in the test is a simple console.log)

@lh0x00
Copy link
Contributor

lh0x00 commented Feb 3, 2020

I'm trying [email protected] available from npm, and it seems to work with node v12 but when I use the --detectLeaks option, every single test fails with a detected leak (even if the only thing in the test is a simple console.log)

can you provide a mini repo to reproduce?

@dylanwulf
Copy link

@lamhieu-vk https://github.com/dylanwulf/jest-25-detect-leak
npm run test: run test with jsdom environment
npm run test:sixteen: run test with jest-environment-jsdom-sixteen
npm run test:node: run test with node environment

When using jsdom or jest-environment-jsdom-sixteen it reports a memory leak even though the only thing in the test is a console log. When using the node environment, it does not report a leak.

@SimenB
Copy link
Member

SimenB commented Feb 3, 2020

@dylanwulf mind opening up a new issue with that? Seems like we're doing something the the globals in jsdom that we don't clean up properly 🙂

@dylanwulf
Copy link

@SimenB #9507

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants