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

Simplifying configuration #7185

Open
SimenB opened this Issue Oct 16, 2018 · 16 comments

Comments

Projects
None yet
8 participants
@SimenB
Copy link
Collaborator

SimenB commented Oct 16, 2018

This is a bit rambling, sorry about that. Feel free to edit this to clean it up and add more to it.

Jest currently has 51(!!) configuration options, many of which overlaps or intentionally overrides other options. (CLI options are out of scope for this issue, but they are obviously very much related)

They fall into a few different categories:

  • file matching
    • some are arrays, some are not.
    • some are globs, some are regexes
      • collectCoverageFrom
      • coveragePathIgnorePatterns
      • forceCoverageMatch
      • modulePathIgnorePatterns
      • testMatch
      • testPathIgnorePatterns
      • testRegex
      • transformIgnorePatterns
      • unmockedModulePathPatterns
      • watchPathIgnorePatterns
  • modules and mocking
    • confusing overlap with node's require api (node paths, extensions)
      • resetMocks vs clearMocks vs restoreMocks
      • automock
      • moduleDirectories vs modulePaths (I honestly have no idea)
  • Coverage things
    • reporters
    • thresholds
    • output directory
  • Setup files
    • confusingly named, and inconsistent if they take array or string (#7119)
  • it keeps going (feel free to edit)

First of all, I'd like to simplify the file matching a lot. Both to make it easier to use, but also easier to implement and reason about.

For file matching I think coveragePatterns: Array<Glob>, testPatterns: Array<Glob>, transformPatterns: Array<Glob> and remove everything else. You can use negated patterns to exclude things instead of a ignore thing. No more force to override ignores.

We could also group things (using the current names, although they are subject to change):

  • coverage can have collectCoverage, coverageDirectory, coverageReporters, coverageThresholds, collectCoverageFrom
  • setup can have globalSetup, setupTestFrameworkScriptFile, setupFiles, globalTeardown.
    • @palmerj3 had a cool idea about having just a single setup which exported different functions. Not as declarative, but maybe better?
  • module can have automock, resolver, moduleDirectories, modulePaths, moduleNameMapper, moduleFileExtensions, resetModules
  • mocks can have resetMocks, clearMocks, restoreMocks, timers (automock?`)
  • snapshots can have snapshotSerializers, snapshotResolver
  • notify and notifyMode can be combined (true is default mode, string sets the mode)

Another thing that's somewhat confusing is the difference between ProjectConfig and GlobalConfig. While the separation makes sense, it's invisible to users, and hard for them to reason about. It also doesn't work well with presets.


Finally, I leave you with this awesome article https://fishshell.com/docs/current/design.html 🙂

Every configuration option in a program is a place where the program is too stupid to figure out for itself what the user really wants, and should be considered a failure of both the program and the programmer who implemented it.

@SimenB SimenB added the Discussion label Oct 16, 2018

@rubennorte

This comment has been minimized.

Copy link
Contributor

rubennorte commented Oct 18, 2018

I completely agree we should rethink our configuration options, so thank you for collecting all this.

Some comments:

For file matching I think coveragePatterns: Array, testPatterns: Array, transformPatterns: Array and remove everything else. You can use negated patterns to exclude things instead of a ignore thing. No more force to override ignores.

I think we should favor regular expressions over globs, as they're more expressive and anything that can be expressed as a glob can be as a regexp. I'd also keep the ignore versions as they're simpler than negated patterns.

We could also group things (using the current names, although they are subject to change):

I'm not sure we should group configuration options in objects. I've usually found flat options easier to deal with and it aligns better with CLI options (e.g.: you could overwrite the collectCoverageFrom config option with a --collectCoverageFrom CLI option).

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Oct 18, 2018

I think we should favor regular expressions over globs, as they're more expressive and anything that can be expressed as a glob can be as a regexp.

The syntax is also way harder, though. I find globs more readable, and easier to write (as you can probably do ls myglob in the terminal to test, that's hardet with regex). Escaping globs are easier as well, I think.

I'd also keep the ignore versions as they're simpler than negated patterns.

As long as we allow an array of globs and apply them in order, it's easy enough to just include an !. Harder with regexes, though.

I'm not sure we should group configuration options in objects. I've usually found flat options easier to deal with and it aligns better with CLI options

I agree on this one, at least depending on how yargs could handle deep objects. If you can do coverage.pattern=blah that's just as easy/readable imo

@jamietre

This comment has been minimized.

Copy link
Contributor

jamietre commented Oct 18, 2018

The syntax is also way harder, though. I find globs more readable, and easier to write (as you can probably do ls myglob in the terminal to test, that's hardet with regex). Escaping globs are easier as well, I think.

Also a fan of globs for file pattern matching. As long as you can pass arrays for all matchers, the limitations vs. regexp seem like not a big deal.

Another alternative (or addition) that would be great is just allowing any file pattern config to also accept (file: string) => boolean as its argument and let the consumer do whatever they want.

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Oct 18, 2018

Glob arrays + functions seems like a nice compromise of ease-of-use vs power to me.

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Oct 18, 2018

Passing a function could definitely work. Would it be inline, or point to some file that exports a function? Both?

Edit: might be confusing that a string can be interpreted as a module, so I guess inline is the way to go

@rubennorte

This comment has been minimized.

Copy link
Contributor

rubennorte commented Oct 18, 2018

Passing a function could definitely work. Would it be inline, or point to some file the exports a function? Both?

It should be a function because otherwise it'd have to be split in two different configuration options (we can't distinguish a string being a glob or a file containing the function).

@jamietre

This comment has been minimized.

Copy link
Contributor

jamietre commented Oct 18, 2018

It should be a function because otherwise it'd have to be split in two different configuration options (we can't distinguish a string being a glob or a file containing the function).

Yes, this seem the most flexible since anyone is always free to just import their function.

Another related discussion here is that a some other config options require that something that's really just a function be implemented as an npm package, e.g. resolver testResultsProcessor, etc.

I'd prefer just allowing config to accept a function for these; people can always implement it as a module themselves if they want to. But it's a lot more cruft to make a module, link it in package.json, etc for these things when I'd rather just import it directly where it's consumed.

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Oct 18, 2018

It's something from when config had to be in package.json. I'm all for saying "use js config". Presets should help keep boilerplate down and if that's an issue for people.

We also have to consider people passing stuff from the CLI, but I don't think that too important for stuff that rarely change (like result processors, resolver, etc).
The use case of setting certain reporters etc on CI only is easily solved in js config by inspecting the env (or using is-ci)

@jamietre

This comment has been minimized.

Copy link
Contributor

jamietre commented Oct 18, 2018

I suppose you could overload again, and accept a string or an fn, where a string is always treated as an npm package? Specifying a reporter by npm package name on the CLI could be useful, though interestingly that can't be done now afaict ;)

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Oct 20, 2018

You can do jest --reporters jest-junit today

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Oct 21, 2018

This is absolutely stupendous. Thank you all for your work here! It's long been a point of confusion for me.

@oleersoy

This comment has been minimized.

Copy link

oleersoy commented Oct 22, 2018

Resuggesting the above linked issue here:

Ideally it would be very simple to configure Jest to use the same glob pattern that typescript uses to resolve paths.

Initially I assumed it had that type of support, but discovered it's regex only per this so question.

If this is supported users can use the same glob pattern in jest that they would use in typescript.

I don't know about the rest of you, but I break out into a cold sweat any time RegEx is mentioned.

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Jan 24, 2019

As part of this, I'd also like to use cosmiconfig to load configuration from files. It'll both allow us to delete code to look for a configuration file, and it'll allow us to support a few more ways to provide config (we currently have jest.config.js, .jestrc and jest in packages.json - all of which are supported by cosmiconfig)

@Farwatch

This comment has been minimized.

Copy link

Farwatch commented Feb 1, 2019

Reposting here at request of @SimenB : #7757

TL;DR: jest.restoreAllMocks() -> jest.restoreAllSpies(), and in config restoreMocks -> restoreSpies

@jeysal

This comment has been minimized.

Copy link
Contributor

jeysal commented Feb 10, 2019

Would be nice to provide a way to specify reporter options (and possibly other things) from the CLI. See #7845

Edit: Sorry, out of scope 😄

@SimenB

This comment has been minimized.

Copy link
Collaborator Author

SimenB commented Feb 10, 2019

From OP:

CLI options are out of scope for this issue, but they are obviously very much related

But yeah, we should figure out a plan. I think as long as we really standardize on how to pass options (I like the reporter:s [name, [name, options]] pattern) it should be possible to figure out some relatively ergonomic way of doing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment