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

fix(jest-haste-map): Fix slowdown on node versions > 10 (second attempt) #8806

Closed
wants to merge 4 commits into from

Conversation

vovkasm
Copy link

@vovkasm vovkasm commented Aug 10, 2019

This is second attempt (first was #8803 and was incorrect, I definitely should run tests)
Fixes facebook/metro#429 for example.

Yes here is #8787, but I will try anyway. This time I split commits to small chunks. I prefer to eliminate any async/await syntax from file not only because it triggers some strange bugs in v8, but mostly because this syntax is ugly and leads to hard-to-understand code while promise based api is more clean (from my point of view).

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@53e964c). Click here to learn what that means.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8806   +/-   ##
=========================================
  Coverage          ?   62.32%           
=========================================
  Files             ?      266           
  Lines             ?    10730           
  Branches          ?     2614           
=========================================
  Hits              ?     6687           
  Misses            ?     3460           
  Partials          ?      583
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.ts 81.39% <79.16%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53e964c...b3e672e. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 10, 2019

Huh, interesting that this seems to fix the OOM we get with circus...

Question - is the issue fixed by just keeping async-await and not transpiling it to generator functions? You can test by just setting our babel config to compile to version 8 and building.

https://github.com/facebook/jest/blob/fafaf893575000ca423dc77aa8aa44554cdb3f57/babel.config.js#L30

@vovkasm
Copy link
Author

vovkasm commented Aug 11, 2019

@SimenB Yes, at least for node 12.8.0 setting target to node: 8 in babel.config.js also fixes issue with metro.

@vovkasm
Copy link
Author

vovkasm commented Aug 11, 2019

I still think that code without async/await looks cleaner and more understandable.

@SimenB
Copy link
Member

SimenB commented Aug 16, 2019

#8787 landed, thanks for the PR (and checking babel config)!

@SimenB SimenB closed this Aug 16, 2019
@github-actions
Copy link

This pull request 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bundleReleaseJsAndAssets slow for react-native 0.60 / node v12 combo
4 participants