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

Remove providesModuleNodeModules from Jest. #8535

Closed
wants to merge 0 commits into from

Conversation

cpojer
Copy link
Member

@cpojer cpojer commented Jun 5, 2019

Summary

With the removal of haste from react-native, we finally do not need providesModuleNodeModules in either Jest or Metro any longer. This is the PR that removes this feature from Jest and a corresponding change will be merged to Metro soon.

This was one of my least favorite configuration options and the added complexity (see the whitelist in jest-haste-map) was annoying to deal with. No more!

Test plan

Ran the tests via yarn jest

@cpojer
Copy link
Member Author

cpojer commented Jun 5, 2019

I'll let you guys merge this on your own time whenever it is convenient for you. I don't wanna conflict with whatever may be happening on master right now :)

@mjesun
Copy link
Contributor

mjesun commented Jun 5, 2019

F I N A L L Y !

🎉🎉🎉🎉🎉

@SimenB
Copy link
Member

SimenB commented Jun 7, 2019

This breaking, but I think we should land it when we're starting to break things for 25. Not sure if disabling the react native test or trying to do some clever yarn tricks to get old dependencies in is the correct solution?

@cpojer
Copy link
Member Author

cpojer commented Jun 7, 2019

I fixed the failures from the initial version of this PR but didn't do anything with the react-native example. I'll leave it to you to decide on the right timing for landing this and what to do with the test. If you temporarily exclude the react-native example, I think that's fine.

@thymikee thymikee added this to the Jest 25 milestone Jun 14, 2019
@SimenB SimenB changed the base branch from master to next August 20, 2019 08:49
@SimenB
Copy link
Member

SimenB commented Aug 20, 2019

I've rebased this and bumped RN from 0.58 to 0.60. Hopefully it passes CI 🙂

@SimenB
Copy link
Member

SimenB commented Aug 20, 2019

image

🙁 Are we still waiting for a RN release that supports this? I don't recall from our discussion

@thymikee
Copy link
Collaborator

@SimenB Haste is fully removed in RN 0.61, the RC should be released soon, maybe even today.

@SimenB
Copy link
Member

SimenB commented Aug 20, 2019

Was just about to paste this:
image

But perfect, let's bump to the RC once it's out then? Or do we wanna wait for stable? I just use Expo, so I have no idea how RN releases work 😛

@thymikee
Copy link
Collaborator

We should be fine with RC once it's out. Expo is always at least 1 release behind for stability reasons + some time to update their modules and the fork I guess. Fun fact – most of the Haste removal work came out from Expo 🙂

@cpojer
Copy link
Member Author

cpojer commented Aug 20, 2019

Yep RC should be fine for testing purposes here.

@SimenB SimenB force-pushed the next branch 2 times, most recently from ff9269b to cf24070 Compare August 22, 2019 06:25
@SimenB SimenB changed the base branch from next to master August 22, 2019 06:58
@SimenB
Copy link
Member

SimenB commented Aug 22, 2019

RC is out, lemme try to rebase

EDIT: Ergh tag is out, but it's not published. https://github.com/facebook/react-native/releases/tag/v0.61.0-rc.0

@SimenB

This comment has been minimized.

@SimenB
Copy link
Member

SimenB commented Aug 28, 2019

As discussed privately, I think we should hold off merging this at least one major - if we do not we will break compatibility with all RN releases prior to 0.61.

@SimenB
Copy link
Member

SimenB commented Feb 11, 2020

We've upgraded RN on master now, so I've rebased this. In theory we're unblocked, and only need to wait until we start landing breaking changes to release

@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #8535 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8535   +/-   ##
=======================================
  Coverage   65.07%   65.07%           
=======================================
  Files         286      286           
  Lines       12144    12144           
  Branches     3010     3010           
=======================================
  Hits         7903     7903           
  Misses       3605     3605           
  Partials      636      636

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 63593a2...63593a2. Read the comment docs.

@kenrick95
Copy link
Contributor

Hi, wanna clarify that this PR means that Jest 26 cannot be used for RN < 0.61 right? Thanks

@SimenB
Copy link
Member

SimenB commented Jun 19, 2020

Correct

@robolivable
Copy link

This option seems to be a work around for an open issue regarding getting jest to run tests in directories nested in folders named node_modules: #2145

Unless I'm missing something this is technically breaking for those users as well... :/

@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 10, 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.

9 participants