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

Support .jestrc #2445

Closed
wants to merge 4 commits into from
Closed

Conversation

stephencookdev
Copy link
Contributor

As described in the comments in #2203, this PR aims to add support for Jest config to be specified in a .jestrc file

Config now attempts this order of priority:

  • --config passed in as a command-line arg (so anyone using an unexpected format, e.g. json or yml, of .jestrc with explicit --config should be unaffected by this update)
  • the result of module.exports from the .jestrc at the cwd
  • the result of package.json .jest at the cwd

And then the last 2 are repeated, but one directory up, until either a non-falsy config is found, or we can't go up a directory anymore.

The reason for checking for a package.json at every directory level is to ensure that this requirement is met:

  • .jestrc in a closer directory to cwd than package.json with config should take priority over package.json
  • .jestrc at same level as package.json should take priority over package.json with config
  • package.json with config in a closer directory to cwd than .jestrc should take priority

Test plan

I'll write some unit-tests once the actual business-logic here is confirmed

But I've tested locally that config-selecting functionality works as expected in the following scenarios:

  • --config followed with no .jestrc or package.json config
  • --config followed with .jestrc and package.json config
  • package.json followed when no --config or .jestrc
  • .jestrc followed when before package.json config
  • `.jestrc followed when after package.json, but package.json has no config
  • package.json used, and .jestrc ignored, when package.json is before .jestrc, and has config

@thymikee
Copy link
Collaborator

@stephencookdev are you still planning to work on this feature?
I wonder if we could add #1206 in a followup PR

@stephencookdev
Copy link
Contributor Author

Yes - I'd forgotten about this a bit tbh

I'm just looking at resolving the merge conflicts and build issue now, and then I'll try and chase people up to review the code.

#1206 in a follow-up PR would be awesome, yeah - I fear it might be a bit more of an involved change than this PR (as this PR is just adding an extra config source, rather than changing the way config gets applied to each test suite); but still worth doing :)

@thymikee
Copy link
Collaborator

Awesome! Thank you

@stephencookdev
Copy link
Contributor Author

@cpojer just pinging for a review in case it fell off your radar :)

I'll try to get some unit-tests up and running for the feature tonight, and having a quick think about what the work to do to get us to #1204 is tonight

But would be great to just have the business-logic of what I've done in this PR confirmed before I get too far into any of that :)

@ArmorDarks
Copy link

Wow, that seems to be great addition! Thanks for your work, @stephencookdev

Hope this PR will be reviewed by @cpojer soon...

@ArmorDarks
Copy link

Ping @cpojer?

Can you at least let us know do you plan to merge it?

Thanks!

@cpojer
Copy link
Member

cpojer commented Mar 13, 2017

Currently I'm undecided. I'm not against it but I don't see the clear value proposition yet. There is also a larger issue about a multi-config runner #2970 which may have implications.

@ArmorDarks
Copy link

Thanks for reply @cpojer!

Usecase is quite straightforward — when you need to dynamically configure certain parts of Jest based on certain conditions, like current environment.

Another important usecase is passing JSPM\SystemJS mappings to Jest directly within configuration file, without need to hack around with additional build steps (which results in quite bad development experience).

Sometimes you do not want to pollute package.json with additional settings. Though, this is more related to preferences of developer.

Finally, by this day it is almost de facto standard to provide possibility of configuration through external rc file.

Just for the information, ESLint uses same approach with configuration file and allows to specify .eslintrc.js which should module.exports configuration object. So it is quite common approach too.

@bookman25
Copy link
Contributor

Another use case I can think of would be to potentially leverage this for the jest-editor-support code. We are currently passing some information through via command line flags to override parameters in the config file. But it would be even easier and scale better over time to just provide a .jestrc file that pulls in the original config file and then augments as necessary.

@ArmorDarks
Copy link

ArmorDarks commented Mar 13, 2017

I reviewed mentioned #2970, but it seems to be related very partially. This PR solves particular issue — it brings current JS configs declaration standards into Jest.

#2970 on contrary sounded to me like an issue about solving particular issues in Facebook repositories (yeap, despite it tries so hard to look like an issue about solving community issue).

Judging from you short descriptions in #2970, addition of rc-files and packages.json configs discovery mechanism does not interfere with #2970 at all, since, as you mentioned, Facebook even do not use packages.json, not to mention rc-files.

@ArmorDarks
Copy link

@cpojer Here is another usecase: #2203

@ArmorDarks
Copy link

@cpojer

I have constructive proposal: since traversing and discovery of configs blocked by #2970, I propose at least to implement .jestrc-related part in separate PR.

In other words, so that --config could accept or JSON, or JS file with module.exports.

Yeap, we will be still forced to use --config .jestrc to point to config file, but at least we finally will be able to write Jest configs in form of pure JavaScript.

@cpojer
Copy link
Member

cpojer commented Mar 13, 2017

--config already supports being passed JSON, fyi.

@cpojer
Copy link
Member

cpojer commented Mar 14, 2017

@bookman25 yea, that's a good point as well; similar to how the multi-config runner may have implications on the design of the .jestrc system. I prefer to hold off on merging a solution too early if we are unsure about how it will work long term, that's the reason this PR hasn't been merged yet. I hope you all understand.

@ArmorDarks
Copy link

@cpojer

--config already supports being passed JSON, fyi.

But it can't support JS files, which module.exports config object. I propose to merge at least this part. Or create standalone PR which will add this functionality. It will make live of many developers much easier.

@cpojer
Copy link
Member

cpojer commented May 2, 2017

This will be in Jest 20, soon.

@cpojer cpojer closed this May 2, 2017
@jokeyrhyme
Copy link

Darn, too late.

May I suggest that we start moving such files out of the project root and into a "config" or ".config" directory instead? I'm imagining a far off utopia where we've reclaimed our project root directories. :)

@cpojer
Copy link
Member

cpojer commented May 3, 2017

This is just a default and it is optional, you can always specify --config=<path>. In fact, that's what we do at Facebook.

@damianobarbati
Copy link

@cpojer it's obliging to use a .json or .js extension anyway, so we can't do --confir=.jestrc to provide whatever file name we want :\

@rfgamaral
Copy link

Yep, same here, getting this:

The --config option requires a JSON string literal, or a file path with a .js or .json extension.
Example usage: jest --config ./jest.config.js

@rfgamaral
Copy link

@cpojer No updates on this?

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
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.

9 participants