CodingZeal / create-react-app

Create React apps with no build configuration.
MIT License
2 stars 2 forks source link

jest rootDir #23

Open manuphatak opened 6 years ago

manuphatak commented 6 years ago

Hello πŸ‘‹

At ThinkCERCA we're experiencing an issue with jest's config. Jest has a optimization tool called jest-haste-map which appears to walk the entire rootDir looking for javascript code. This includes node_modules from old frontend code in a different directory, it includes javascript included with ruby in our vendor directory. And on our CI server, it includes everything cached by npm & yarn in thinkCERCA/.semaphore-cache. As you might imagine, there's all sorts of clashes happening. For whatever reason, certain snapshot tests reliably fail on our CI server.

I found 2 solutions:

  1. Delete npm/yarn cache on our CI server.

  2. Change jest's rootDir from thinkCERCA/ to thinkCERCA/client

Since create-react-app limits our ability to customize our configuration, solution 2 required a little bit of hackery that's worth sharing.

I copied in @zeal/react-scripts/scripts/test.js and @zeal/react-scripts/scripts/utils/createJestConfig.js to our codebase and made some changes to move jest's rootDir. I posted a diff of the changes here: https://gist.github.com/bionikspoon/af66c22122dc19f57ebf0e04b0137935

About half the changes are a product of copying the file over to our local project directory, the other half are for moving jest's rootDir.

These changes may or may not belong in your repo, you decide. If they are, happy to help any way I can.

randycoulman commented 6 years ago

@bionikspoon Thanks for reporting this. It's an interesting problem, for sure.

TLDR: It might be worth opening this issue upstream on facebook/create-react-app. They may not want to fix it there, but either way, there's some work being done upstream that's going to have an impact on a solution for this problem, so we're going to wait until that work is available to us to see if that resolves the issue. We'll keep this issue open to remind ourselves to come back and check once we move to CRA 2.0.

Longer version:

It seems like this is an issue with the upstream CRA itself. I suspect that if you used the original create-react-app in your repo, it would have the same problems as this fork. I think it would be worth opening an issue on that repository to see what they say about it.

However, I think that CRA expects to own the entire repository where the app is generated, so they may deem this as something they don't want to fix. I might be wrong about that, but I think that would be a reasonable response on their part.

Since this fork is intended to be embedded within a parent app, it may fall to us to address this issue. The diff you provided seems reasonable in that light, with one minor concern:

What information does jest/jest-haste-map get from having the base project directory as its <rootDir> that it wouldn't get by narrowing <rootDir> down to just the client directory (or src directory in the upstream project)? That is, are we subtly breaking something by making this change?

In digging into this problem, I noticed that there's a bunch of work being done upstream to support lerna/yarn workspaces, and that work will have an impact on what our solution might look like. That work might even fix this issue as a side-effect. See https://github.com/facebook/create-react-app/pull/3741 for example.

Because of that, we're going to hold off on addressing the issue here for now.

We'll keep this issue open and revisit after we can upgrade to CRA 2.0.

bmatto commented 6 years ago

@randycoulman Thank you for digging into this, very much appreciate the insights. As @bionikspoon mentioned the issues related to this are preventing snapshot testing πŸ“Έin our app. To that end our solution in the interim is going to be to pull these test scripts in locally to our project as seen in the diff provided.

Again, super appreciative of the time spent looking at this. We'll keep on eye for CRA 2.0 as I'm sure you all will as well.

manuphatak commented 6 years ago

I second everything @bmatto said.

Thank you @randycoulman for looking into this so quickly and thoroughly. It is appreciated! πŸ˜„