Enable src/node_modules

[modernserf]

[HOLD] not particularly useful without changes in Jest

Partially fixes#607

These are the necessary changes in react-scripts to allow code to live in src/node_modules and allow Jest to run tests in that directory.

However, Jest cannot find these tests on its own -- all files in anynode_modules are filtered out even before the file matching patterns are applied. See issue jestjs/jest#2145.

Without changes in Jest, there are a few workarounds:

Import the package tests into a file that jest can find

// src/node_modules/Foo/Foo.test.jsit("does a thing",()=>{expect(true).toBe(true)})

// src/index.jsimport"Foo/Foo.test.js"

Whitelist the directories you want to test with the haste.providesModuleNodeModules option

This is not the intended use for this option, but its used as a whitelist for the haste module resolution, and therefore Jest can find tests in these directories.

TODO

• get related issue in facebook/jest resolved
• figure out how to test this
• rebase, clean up etc

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

[facebook-github-bot]

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

[julien-f]

I also love putting application-wide packages in a node_modules directories but it does not seem standard and encouraged by the community which generally uses other solutions such as:

• non standard bundler configuration
• extraneous linking step (like lerna bootstrap)

I find it sad that we have to find other, more complex solution, while the existing resolution algorithm is enough :/

[gaearon]

What is the resolution now that jestjs/jest#2145 is closed?

[gaearon]

This is becoming sort of critical because we want to remove NODE_PATH support due to issues like #1023.

[julien-f]

As far as I can see, the issue as been closed as wontfix, not resolved :/

I also created an issue for this (jestjs/jest#1995) in which @cpojer answered me by saying that he would not oppose a fix but he does not have time to take care of it himself.
Unfortunately I do not have the time to it either.

[gaearon]

I’m sure we can all persuade @cpojer. I can literally bug him all day until he agrees to merge a PR as he moved to the London office. But we need somebody willing to figure out the problem and the solution and submit said PR.

[cpojer]

I'll do anything for you Dan but I may ask you for a favor in the future.

[gaearon]

A day of work on a Jest issue of your choice?

[modernserf]

I updated the PR with the haste: { providesModuleNodeModules: [".*"] } hack, which works, but generates a bunch of spurious error noise on the first test run, which seems like a non-starter.

Also, I still have no idea how i'm supposed to write an automated test for this, or if there even are tests in create-react-app (?!)

[gaearon]

It's sort of unfortunate that this will end up in config after ejecting.
(@cpojer do we have better options?)

[gaearon]

Also, I still have no idea how i'm supposed to write an automated test for this, or if there even are tests in create-react-app (?!)

No, we only do artisan tests. On a serious note, if Travis passes it means that a basic project runs webpack and jest correctly without crashing, but you can't automatically test a particular feature right now.

[shanewilson]

Hey @gaearon@cpojer@modernserf,

I've been working on a monorepo build tool that works with yarn and supports app-level modules under node_modules so I would be happy to help out with config settings or problem/solutions I've run in to while trying to integrate outside tools.
I've found most of the friction that comes with app-level node_modules is just the heavy handed approach people take to ignoring */node_modules/* rather than just /node_modules

https://github.com/knitjs/knit/tree/master/modules/node_modules/%40knit/jest-config-socks
https://github.com/knitjs/knit
https://github.com/boennemann/alle

[cpojer]

To give a quick overview:

• I didn't initially realize that this is a CRA issue. I'm happy to make Jest work well for use-cases that CRA supports (within reason, of course).
• This is all done inside of jest-haste-map and it has 100 % test coverage inside of Jest. If you make a change there and add a good test, we'll be all good.
• jest-haste-map does static analysis for Jest and supports FB's module system (this second part is irrelevant for this issue). It fetches a list of all files in the system and extracts metadata like dependencies (for reverse lookup during jest --watch so we only run the affected tests) and stores them in a cache.
• It parses all JS files and package.json files except those in node_modules which we actually just skip and don't even put into the virtual file system. This means when Jest is trying to match the testRegex against the virtual file system (HasteFS), it can't find the tests. This is the source of our problems. There is an option retainAllFiles in jest-haste-map that keeps those files around, which may or may not work for this use case. I'm not entirely clear what the correct fix should look like, only that we should definitely support this use case. Please feel free to try some things and send a PR.
• While you are changing Jest's haste map, always have npm run watch running in the Jest checkout and run Jest with --no-cache to make sure you are hitting the right code paths in the haste map.
• source

Let me know if you need anything else.

[modernserf]

@cpojer just got some time to work on this again. I have some changes in jest that work for create-react-app but have a failing test and I don't understand Jest well enough to know why. Can you help me understand?

[robertvansteen]

How's the progress on this? I'd love to help because this is kind of a blocking issue for me now.

[modernserf]

@rovansteen get the tests passing on this: modernserf/jest@a687e26

[gaearon]

@modernserf I think it's best to send a PR to start discussion for better visibility.

[Reanmachine]

Can I make a general inquiry about what's going on here? The best I can tell is that @modernserf took this under his wing but due to the way haste-map is set up in jest this can't be done without some rather complex changes (the jest pr from @modernserf looks completely stonewalled).

I guess what I'm asking is, with all the work around this being prefixed with "Don't work on this, it's being taken care of" but looking like we're stuck until someone (??) fixes haste-map with a non-trivial feature for white-listing nothing will come of this.

There's been no activity for 22 days on any of the issues/pr's related to this. I'd love to try and help on the jest/haste-map side but being that it's new functionality required I don't think it's a great candidate for someone unfamiliar with the jest code base or not in-the-loop with their development goals.

How can I help? Or should I transition my projects off the src/node_modules path?

[modernserf]

@Reanmachine the haste map thing will probably not be fixed per se -- I think the idea was that someone should add a testPathIgnorePatterns config option that actually works with node_modules as documented. That someone could be you!

[gaearon]

Closing. I think we want to have good integration with Yarn Workspaces and/or Lerna instead.

[cecilemuller]

That's unfortunate as the whole point of the src/node_modules structure is to avoid additional config and dependencies by using the native node module resolution.

[gaearon]

I understand, but it's not the approach the ecosystem is embracing.