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

Enhance Jest config error for setupTestFrameworkScriptFile #3512

Merged
merged 4 commits into from
Jan 17, 2018

Conversation

jackfranklin
Copy link
Contributor

I wasn't aware of the fact that users of c-r-a could just define
src/setupTests.js and it would be configured with Jest - I nearly
ejected before I found a GitHub issue that confirmed this functionality.

I thought it might be a nice idea to add it to the error about Jest
config overrides to stop others ejecting when they don't need to.

screen shot 2017-11-28 at 07 15 00

I wasn't aware of the fact that users of c-r-a could just define
`src/setupTests.js` and it would be configured with Jest - I nearly
ejected before I found a GitHub issue that confirmed this functionality.

I thought it might be a nice idea to add it to the error about Jest
config overrides to stop others ejecting when they don't need to.
@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Unless I read the whole message, a quick skim makes it seem like the option is not supported at all and I need to eject. So I don't think this wording makes it much better. :-)

Maybe we could tweak the wording?

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Let's tweak the message so it's more obvious what to do on the first glance

@jackfranklin
Copy link
Contributor Author

👍 sounds good, do you have any thoughts on what it should be?

Maybe we should re-organise it to first show my additional message about the setup file; and then have the rest of the message? That way the first thing the user reads is "hey you can use setupTestFile!" rather than "no this doesn't work".

What do you think? :)

@gaearon
Copy link
Contributor

gaearon commented Jan 11, 2018

Yep

@jackfranklin
Copy link
Contributor Author

screen shot 2018-01-11 at 18 28 15

How does that look?

@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2018

  • Let's not show the other part of the message to keep it focused (no need to mention other options).
  • Looks like the option name is formatted inconsistently (backticks, no backticks, bold). Please use bold for it.

@jackfranklin
Copy link
Contributor Author

@gaearon thanks for your help :)

I've now changed the messages as you suggested.

For setupTestFrameworkScriptFile:

screen shot 2018-01-17 at 12 53 00

For any other config that isn't supported:

screen shot 2018-01-17 at 12 55 22

@gaearon gaearon added this to the 2.0.0 milestone Jan 17, 2018
@gaearon gaearon changed the base branch from master to next January 17, 2018 13:26
@gaearon gaearon merged commit 7969b48 into facebook:next Jan 17, 2018
@jackfranklin jackfranklin deleted the setup-test-file-error-help branch January 18, 2018 08:34
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
…k#3512)

* Enhance Jest config error for `setupTestFrameworkScriptFile`

I wasn't aware of the fact that users of c-r-a could just define
`src/setupTests.js` and it would be configured with Jest - I nearly
ejected before I found a GitHub issue that confirmed this functionality.

I thought it might be a nice idea to add it to the error about Jest
config overrides to stop others ejecting when they don't need to.

* Change the order of Jest config errors.

* Show different error for `setupTestFrameworkScriptFile`

* Tweak the message
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
…k#3512)

* Enhance Jest config error for `setupTestFrameworkScriptFile`

I wasn't aware of the fact that users of c-r-a could just define
`src/setupTests.js` and it would be configured with Jest - I nearly
ejected before I found a GitHub issue that confirmed this functionality.

I thought it might be a nice idea to add it to the error about Jest
config overrides to stop others ejecting when they don't need to.

* Change the order of Jest config errors.

* Show different error for `setupTestFrameworkScriptFile`

* Tweak the message
@lock lock bot locked and limited conversation to collaborators Jan 20, 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