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

Allow projects to customize Jest configs. Fixes #338. #355

Closed
wants to merge 1 commit into from

Conversation

cpojer
Copy link
Contributor

@cpojer cpojer commented Aug 4, 2016

This changes create-jest-config to take in the app's configuration so that people can decide to overwrite the defaults if they like.

I recommend people to use the node environment and not bother with jsdom, however people might have existing code that accesses DOM APIs. In such cases people can put

"jest": {
  "testEnvironment": "jest-environment-jsdom"
}

into their config and npm run test will merge the default config and the app config together. If someone decides to eject, the custom config will be kept in-tact when we write the default config into package.json.

I was unsure how to test my changes to test/eject so I figured I'll add a Jest test to scripts which can be invoked using npm run test-scripts. I used a snapshot test to lock in the Jest config and validated that it is correct. I also enjoy how meta it is to use snapshots in Jest to test a feature for Jest integration into a project that is promoting snapshot testing. Anyway, people can now write tests for stuff in scripts/ to make sure we don't regress.

Fixes #338

@ghost ghost added the CLA Signed label Aug 4, 2016
@mxstbr
Copy link
Contributor

mxstbr commented Aug 4, 2016

I'm not a big fan of this change for the same reasons we don't want to provide webpack configuration overrides. Not sure why somebody would have existing code that uses DOM APIs in a newly created app?

@cpojer
Copy link
Contributor Author

cpojer commented Aug 4, 2016

One reason I mentioned in the associated task is that many people still prefer using enzyme and might want to use it together with create-react-app, so they should get to do that. I believe the test renderer is the future of React testing but we don't have a selector API yet, unfortunately (it is being worked on, though). Providing an escape hatch to modify the configuration seems valuable. There isn't much of a point in forcing them to eject just to add some configuration for their test environment, right?

I didn't realize you aren't allowing any other configuration overrides. Happy to follow the philosophy of this project but personally I don't want anyone to feel locked in, which is why I'm providing this solution. Another solution would be a soft-eject which simply copies the default jest config into package.json and the test command would then pick it up from there. We could also just ignore the default config if the user provides his own: appPackage.jest || createJestConfig(…) but then they need to copy the default config over manually, which might be confusing.

@mxstbr
Copy link
Contributor

mxstbr commented Aug 4, 2016

There isn't much of a point in forcing them to eject just to add some configuration for their test environment, right?

That's what we do for every other configuration and tool too, not sure why the test env would be different tbh.

I didn't realize you aren't allowing any other configuration overrides. Happy to follow the philosophy of this project but personally I don't want anyone to feel locked in, which is why I'm providing this solution. Another solution would be a soft-eject which simply copies the default jest config into package.json and the test command would then pick it up from there.

We've had these discussions in a few issues and PRs now but I can't find them (I'm sure @gaearon has them stored somewhere), basically you either like create-react-app or you eject. We don't want to introduce any custom config or soft ejection to avoid dependency issues and tool lockins down the road. That's been a pretty harsh line we haven't gone over at all. 😊

That's also why including Jest by default has stirred such a big discussion

@cpojer
Copy link
Contributor Author

cpojer commented Aug 4, 2016

Got it, so the way I see it, we now have two options:

  • Keep the node test environment. If people want to use jsdom, they should be encouraged to load jsdom themselves in Jest. We can provide documentation on the Jest website, I'm happy to update the tutorials to mention CRA. This option is definitely preferred for performance reasons, however it might be harder for a beginner to understand what to do if they want to break out of it. I think good documentation will take care of this for the most part.
  • Switch to jsdom by default. This has a 500ms startup overhead when starting Jest (it's a one time cost per invocation, not per test file) but adopting enzyme might be less work.

I'm inclined to go with option 1, because that's really where I see things heading. What do you think?

@mxstbr
Copy link
Contributor

mxstbr commented Aug 4, 2016

however it might be harder for a beginner to understand what to do if they want to break out of it.

I totally agree, I don't think beginners will download CRA and immediately start thinking "Oh, I need jsdom for enzyme to test my react app!".

Let's do it!

@cpojer
Copy link
Contributor Author

cpojer commented Aug 4, 2016

I'm sorry, with the part that you quoted and your comment it feels inconclusive whether you are voting for option 1 or 2. Which is it?

@mxstbr
Copy link
Contributor

mxstbr commented Aug 4, 2016

Sorry, Option 1 sounds good to me! 👍

@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

I don't think beginners will download CRA and immediately start thinking "Oh, I need jsdom for enzyme to test my react app!".

I actually sort of expect that. Also many user written components reference things like window.

I think shipping jsdom by default might be a better decision until TestRenderer is more fully fledged.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
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.

3 participants