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

jest-haste-map: Fixed Haste whitelist generation for scoped modules on Windows #6980

Merged
merged 3 commits into from
Sep 17, 2018

Conversation

empyrical
Copy link
Contributor

Summary

This PR modifies the function getWhiteList in jest-haste-map to handle flipping the path separator on @scoped/modules on Windows. This allows Haste to properly resolve JS files inside of scoped modules on Windows.

Needed to resolve facebook/metro#241 and facebook/metro#249

Test plan

Using a version of jest-haste-map patched with this patch, and the test suite in my Metro pull request, all tests pass

@empyrical empyrical changed the title jest-haste-map: Fix scoped providesModuleNodeModules on Windows jest-haste-map: Fixed Haste whitelist generation for scoped modules on Windows Sep 15, 2018
@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Could you add a test? We run CI on both linux and windows, so we should be able to get coverage for it 🙂

@empyrical empyrical force-pushed the jest-haste-map-scoped-modules branch from 3e2900a to 5861d41 Compare September 15, 2018 17:25
@empyrical
Copy link
Contributor Author

empyrical commented Sep 15, 2018

Right now the entire jest haste map suite is skipped on Windows:

https://github.com/facebook/jest/blob/master/packages/jest-haste-map/src/__tests__/index.test.js

Making it so it's split out into a posix and win32 section like Metro does here is a good idea that I'd like to do:

https://github.com/facebook/metro/blob/master/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-integration-test.js

But such an effort would be an entire separate PR's worth of effort, IMO. Especially since a lot of the util functions use posix separators, I'd need to fix them to use platform-specific path separators

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Right now the entire jest haste map suite is skipped on Windows:

Aww, that's too bad :(

Help with having it run on Windows would be greatly appreciated, none on the team uses it

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Note that we have landed a breaking change on master, so unless @mjesun wants do do some branching and cherry-picking, this fix won't be released for some time

@empyrical
Copy link
Contributor Author

I see, I will see if I can make things work on Metro's end until then

@SimenB SimenB requested a review from thymikee September 15, 2018 18:03
@rafeca
Copy link
Contributor

rafeca commented Sep 15, 2018

so unless @mjesun wants do do some branching and cherry-picking, this fix won't be released for some time

@mjesun pleazzzzzeee ☺️

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Heh. Easiest is probably to revert #6960, release, then revert the revert

@rafeca
Copy link
Contributor

rafeca commented Sep 15, 2018

I'm actually really interested in having #6960 released as well haha, so whatever you folks decide is good for me 😄

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Yeah, but that's a breaking change, and we want to do a few more at the same time (which are not ready to land). Maybe we can do a pre-release? I don't know if you'd wanna use that in metro or not

@mjesun
Copy link
Contributor

mjesun commented Sep 17, 2018

@SimenB Yeah, let's do a pre-release. Feel free to merge as many breaking stuff as wanted .:)

@SimenB SimenB merged commit fb20ddd into jestjs:master Sep 17, 2018
empyrical referenced this pull request in status-im/react-native-desktop-qt Oct 6, 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.

Haste has troubles resolving modules inside @scoped modules
5 participants