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 support for @providesModule #6104

Merged
merged 4 commits into from
Oct 10, 2018

Conversation

rubennorte
Copy link
Contributor

Summary

@providesModule is an undocumented Jest feature that some packages (mostly internally at Facebook) used to use. Most of this packages don't use it anymore, so we can stop supporting it by default and enable it via hasteImplModulePath if anyone requires it (it receives the filename, so it can read the file to extract any @providesModule annotation).

This will also allow jest-haste-map to stop reading the contents of the files if consumers don't need the dependency list (like Metro). This should be implemented as an option (e.g. extractDependencies: boolean) in a following PR.

Test plan

All Jest tests have been updated so running them and building the project should be enough.

@mjesun
Copy link
Contributor

mjesun commented May 1, 2018

Let's not merge that one until I finish upgrading internally. There's too much flakiness already and I don't want to deal with more.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

This looks great, very nice work! Can you make sure react-native open source (which has a jest-preset.json in the react-native-github folder) is properly set up to extract haste names same as Metro (if it isn't already)

@mjesun this shouldn't affect FB as we use a custom hasteImpl already.

@mjesun
Copy link
Contributor

mjesun commented May 1, 2018

The more commits we add, the harder is to discover what breaks. And it's already quite hard.

@rubennorte rubennorte force-pushed the remove-providesmodule-support branch from 954713a to ee36091 Compare May 1, 2018 16:40
@rubennorte
Copy link
Contributor Author

rubennorte commented May 1, 2018

@cpojer thanks! React Native, Relay and Draft.js already have a hasteImpl file and don't contain @providesModule anymore 😄

@cpojer
Copy link
Member

cpojer commented May 1, 2018

Yes, that's true for React Native but not for the Jest config created for React Native projects, see https://github.com/facebook/react-native/blob/master/jest-preset.json#L2-L8 which should have a hasteImpl specified.

@rubennorte
Copy link
Contributor Author

rubennorte commented May 1, 2018

@cpojer oh, for projects using React Native. Got It! Thanks!

@rubennorte rubennorte force-pushed the remove-providesmodule-support branch 2 times, most recently from a6c03ea to 5c45921 Compare May 2, 2018 11:15
@rubennorte
Copy link
Contributor Author

Integration tests are failing because the React Native version that works without @providesModule hasn't been released yet.

@mjesun
Copy link
Contributor

mjesun commented May 3, 2018

@rubennorte understood, however merging this would make all builds crash. Can we duplicate or make a patch to the existing RN tests?

@cpojer
Copy link
Member

cpojer commented May 4, 2018

You could make the RN tests pass by providing a custom hasteImpl for now and then upgrade the dep later, right?

@chirag04
Copy link

chirag04 commented May 8, 2018

fbjs is still using @providemodules: https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/__forks__/invariant.js#L7

This is probably causing e2e tests to fail on the react-native repo. The custom hasteModule implementation in RN does not handle prodvidemodule annotations inside node_modules directory. I'm not sure what's the best way to move forward. maybe upgrade fbjs and handle providemodules inside node_modules?

@mjesun
Copy link
Contributor

mjesun commented May 8, 2018

cc @rubennorte ^

@hramos
Copy link
Contributor

hramos commented May 16, 2018

Thanks to @chirag04 for the heads up. I'm looking into a failing snapshot test in React Native when Jest 23.0.0-charlie.2 is used, which may be related to this issue (I'm still investigating): facebook/react-native#19308

Any ideas on how to move forward?

@rubennorte
Copy link
Contributor Author

rubennorte commented May 22, 2018

@chirag04 even though we're removing support for @providesModule, specifying providesModuleNodeModule makes that directory analyzable by HasteMap. If we then provide a hasteImpl that extracts the module name from the filename, we should be able to use those modules as we've been doing so far.

I'm pretty sure the e2e errors you're pointing out aren't caused by this change but by an issue when not using Watchman.

@SimenB
Copy link
Member

SimenB commented May 22, 2018

As far as I can tell, all the test failures for RN (in the linked issue, at least) are due to #6162

@fatfatson
Copy link

the latest react-native source has removed @providesModule and use a hasteImpl script to parse filename for haste module name, all my jest test cases are failing due to the could not find module error:
https://stackoverflow.com/questions/50722668/jest-could-not-found-haste-module-when-test-react-native-component

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@thymikee
Copy link
Collaborator

thymikee commented Aug 7, 2018

@rubenmoya @mjesun @hramos what's the status of this diff?

@rubenmoya
Copy link
Contributor

I'm pretty sure you meant to ping @rubennorte 😂

@thymikee
Copy link
Collaborator

thymikee commented Aug 7, 2018

Damn you GitHub autocomplete! 😅

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Sep 17, 2018

@rubennorte any news on this? Next release is a major, so would be nice to land in it

@rubennorte
Copy link
Contributor Author

@SimenB I'm working on some changes to jest-haste-map. I'll work on this again once I finish that.

@rubennorte rubennorte force-pushed the remove-providesmodule-support branch 3 times, most recently from 8c902d4 to 6631437 Compare October 10, 2018 15:06
@rubennorte rubennorte changed the title Remove default support for @providesModule Remov support for @providesModule Oct 10, 2018
@rubennorte rubennorte changed the title Remov support for @providesModule Remove support for @providesModule Oct 10, 2018
Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov-io
Copy link

Codecov Report

Merging #6104 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6104      +/-   ##
==========================================
+ Coverage    66.6%   66.63%   +0.03%     
==========================================
  Files         253      253              
  Lines       10629    10617      -12     
  Branches        4        4              
==========================================
- Hits         7079     7075       -4     
+ Misses       3549     3541       -8     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-haste-map/src/worker.js 84.05% <ø> (+6.28%) ⬆️
packages/jest-haste-map/src/blacklist.js 100% <ø> (ø) ⬆️
packages/jest-haste-map/src/index.js 87.53% <100%> (+0.27%) ⬆️

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 179b3e8...5a4f220. Read the comment docs.

@rubennorte rubennorte merged commit a1b82ca into jestjs:master Oct 10, 2018
@rubennorte rubennorte deleted the remove-providesmodule-support branch October 11, 2018 09:27
@SimenB
Copy link
Member

SimenB commented Oct 18, 2018

@rubennorte thoughts on the test added in #6687? As far as I can tell it only works because it depends on @providesModule. I'm having issues with it in #7016, which is why I'm asking 🙂

@rubennorte
Copy link
Contributor Author

@SimenB I think that test is wrong right now because the local invariant isn't registered as a module anymore. Even though we don't have support for @providesModule we can create modules from local files via hasteImplModulePath. I'll modify that test to use it an make sure it tests the right thing. Not sure what are the problems you're having with it. Can you add more information in that PR so I can help you?

rubennorte added a commit to rubennorte/jest that referenced this pull request Oct 18, 2018
rubennorte added a commit to rubennorte/jest that referenced this pull request Oct 18, 2018
rubennorte added a commit to rubennorte/jest that referenced this pull request Oct 18, 2018
rubennorte added a commit to rubennorte/jest that referenced this pull request Oct 18, 2018
@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 12, 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.