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

Cleanup Jest config #6654

Merged
merged 36 commits into from
Apr 4, 2019
Merged

Cleanup Jest config #6654

merged 36 commits into from
Apr 4, 2019

Conversation

ianschmitz
Copy link
Contributor

@ianschmitz ianschmitz commented Mar 15, 2019

General cleanup after the Jest 24 PR.

This also includes jsdom@14 via https://www.npmjs.com/package/jest-environment-jsdom-fourteen. Since we have a node >= 8.10 requirement, we are able to use the latest version of jsdom which includes additional implementations of browser APIs such as MutationObserver (which we had an issue filed for over at #6617).

/cc @SimenB. Is there more you recommend we do to cleanup our Jest config for TypeScript?

@SimenB
Copy link
Contributor

SimenB commented Mar 15, 2019

WRT TypeScript, I think you're good (Jest by default finds ts and tsx, but you have more opinionated paths on top of its defaults). Other things I noticed though:

setupFiles: [
isEjecting
? 'react-app-polyfill/jsdom'
: require.resolve('react-app-polyfill/jsdom'),
],

Ditch isEjecting condition - we want require.resolve no matter what and not rely on the package manager hoisting things

I'd prefer require.resolve('jest-environment-jsdom'); and add a dependency on it, but Jest has dependencies on jest-environment-{jsdom,node} so it'll be able to resolve these on its own. So you might not want that, although that means jest might load some other version of the env than CRA expects. Not sure if that's a feature or not

require.resolve('babel-jest'); instead of making assumptions about where in node_modules it resides

moduleNameMapper: {
'^react-native$': 'react-native-web',
'^.+\\.module\\.(css|sass|scss)$': 'identity-obj-proxy',
},

require.resolve at least on identity-obj-proxy. Maybe not RNW as that's up to the user and it should always be in the top level so Jest should be able to resolve irregardless of package manager hoisting

@ianschmitz
Copy link
Contributor Author

Thanks a ton @SimenB. Great feedback. I'll incorporate your recommendations shortly.

Would you recommend using using https://github.com/theneva/jest-environment-jsdom-thirteen for testEnvironment as mentioned in the Jest docs? I imagine it would help with issues like #6617 since they've recently added support for MutationObserver. We have a node 8 requirement for CRA now anyways so I can't really see a downside?

@SimenB
Copy link
Contributor

SimenB commented Mar 15, 2019

Would you recommend using using theneva/jest-environment-jsdom-thirteen for testEnvironment as mentioned in the Jest docs?

JSDOM 14 has come out, so I'd probably jump straight to that (not sure if anyone's released a jest environment for it yet, though). Good call 🙂

@ianschmitz
Copy link
Contributor Author

JSDOM 14 has come out, so I'd probably jump straight to that (not sure if anyone's released a jest environment for it yet, though). Good call 🙂

Working on it 😉

https://github.com/ianschmitz/jest-environment-jsdom-fourteen

Copy link
Contributor

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

You might wanna update to jest 24.6, it has a few perf improvements

packages/react-scripts/scripts/utils/createJestConfig.js Outdated Show resolved Hide resolved
@ethanneff ethanneff mentioned this pull request Apr 4, 2019
@iansu iansu merged commit 76fea02 into facebook:master Apr 4, 2019
@ianschmitz ianschmitz deleted the jest-cleanup branch April 5, 2019 04:28
@lock lock bot locked and limited conversation to collaborators Apr 10, 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.

4 participants