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 jest inline snapshots to update without affecting the whole file #5807

Open
mmkal opened this issue Jan 28, 2019 · 16 comments
Open

Allow jest inline snapshots to update without affecting the whole file #5807

mmkal opened this issue Jan 28, 2019 · 16 comments
Labels
area:ranges Issues about formatting/ignoring/etc segments of files help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue!

Comments

@mmkal
Copy link
Contributor

mmkal commented Jan 28, 2019

The community seems to strongly agree that prettier shouldn't allow configuration: #40

That decision is a fine one; it probably makes the library easier to maintain and make it do its job well. In the end it's meant I've never actually been on a team willing to use prettier because some of the rules just didn't look right, and tools like tsfmt were just about good enough. All of that is fine, this is a deliberately opinionated tool. Or rather, it was fine, until jest inline snapshots came along. They use prettier to reformat the entire file when adding a new snapshot.

For a team like mine that doesn't want to use prettier, this can be quite jarring. We end up having to duplicate our tslint and tsfmt configs in prettier, as far as possible, just to stop the various tools fighting each other. But prettier has a lot of other opinions that cause unexpected and unwanted changes in files with snapshots.

It'd be really nice if prettier's rules/transformations could be disabled (not configured - just turned off), so that there could be a change-nothing mode which won't cause large diffs when adding inline snapshots to tests files. Outside of jest snapshots, it may also be good for adoption - if a team likes all but one of prettier's rules right now, the only option is to avoid the tool completely. Would it be possible/easy to have an off switch for each rule, without compromising the opinionated philosophy?

@SimenB
Copy link
Contributor

SimenB commented Jan 28, 2019

An improved range api might make this less invasive: jestjs/jest#6380 (comment)

@lydell
Copy link
Member

lydell commented Jan 28, 2019

@mmkal Prettier doesn't really work that way. Basically, there are no "rules" that can be turned on or off. Prettier is just one big transform that eats your code and spits out new code. So implementing your suggestion would be difficult.

Am I understanding your right that you are not specifically looking for this feature, but rather suggesting it as a solution to your real problem – Jest inline snapshots reformatting your entire file in unwanted ways? If so, would it be OK to change the scope of this issue to just that? Probably, it would mean improving the range API as @SimenB mentioned.

@lydell lydell added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jan 28, 2019
@mmkal
Copy link
Contributor Author

mmkal commented Jan 28, 2019

@lydell yes, I'll change the title from "Allow turning some/all rules off" to "Allow jest inline snapshots to update without affecting the whole file".

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jan 28, 2019
@mmkal mmkal changed the title Allow turning some/all rules off Allow jest inline snapshots to update without affecting the whole file Jan 28, 2019
@duailibe
Copy link
Member

duailibe commented Jan 28, 2019

This makes totally sense but this issue should be in https://github.com/facebook/jest instead.

@duailibe duailibe added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jan 28, 2019
@mmkal
Copy link
Contributor Author

mmkal commented Jan 28, 2019

I got the impression that it wasn't possible or was prohibitively difficult with the current range API. @azz is that right?

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jan 28, 2019
@azz
Copy link
Member

azz commented Jan 29, 2019

I remember struggling with getting the Range API working. jestjs/jest#6380 (comment)

The issue with running prettier.format() with ranges multiple times is the return value from format() is obviously different from the input, so you need to re-calculate (re-parse) the file to get ranges for the second format(), and so on.

@lydell
Copy link
Member

lydell commented Jan 29, 2019

@azz Could you use the good old trick of applying the changes backwards, which might keep ranges intact?

@azz
Copy link
Member

azz commented Jan 30, 2019

@lydell That's an interesting idea... Makes sense in my head, would like to see it implemented.

@alexander-akait alexander-akait added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Aug 8, 2019
@lydell lydell added help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Aug 23, 2019
@sheerun
Copy link
Contributor

sheerun commented Aug 28, 2019

I tried to use the same approach as https://github.com/nrwl/precise-commits (which is neither maintained nor working), and create patches with diff-match-patch. The result was hours of struggling and many destroyed files because patching this was is very fragile (and not performant).

I think the only solution is to employ backward trick you're describing, or to make prettier support multiple cursors and employ this idea: #4926 (comment)

Also I think to make this operation reasonably performant and avoid calling format multiple times, API should be changed to accept multiple ranges, for example:

format(source, ranges: [{ start: 10, end: 10 }, { start: 15, end: 16 }])
prettier --ranges 10-10,15-16

Unfortunately in current format API for formatting ranges is not very usable or stable

@Bastczuak
Copy link

Hello,

is there a possibility that toMatchInlineSnapshot can be configured to use just eslint instead of prettier in the future?

@nemoDreamer
Copy link

Prettier does allow for minimal configuration, in a number of ways, most notable via standard named config files: https://prettier.io/docs/en/configuration.html

Is there any reason (especially given the existence of Jest's prettierPath config key) to not use a project's prettier configuration if one is present?

We've got a project using Prettier, but we still use 4 spaces instead of 2, and Jest reflows the entire file with each inline snapshot update... 😢

@nemoDreamer
Copy link

is there a possibility that toMatchInlineSnapshot can be configured to use just eslint instead of prettier in the future?

We're running our Prettier via ESLint, to avoid conflicts between both "recommended" sets.
But our ESLint-run Prettier reads from its own .prettierrc file, just like Jest could as well.

@nemoDreamer
Copy link

When I set prettierPath to point to our local version of prettier:

prettierPath: '../../node_modules/.bin/prettier',

I get the following on u to update inline snapshot:

  ● Test suite failed to run

    TypeError: Invalid Version: undefined

      at new SemVer (../../node_modules/jest-snapshot/node_modules/semver/semver.js:314:11)

@lydell
Copy link
Member

lydell commented Apr 21, 2020

@nemoDreamer For me, my Prettier config does seem to get picked up by Jest: https://github.com/lydell/jest-prettier-test

@lsirivong
Copy link

For anyone else landing here from a search: as of jest 27.0.0 prettier is only used for inline snapshots if it's found in your project: https://github.com/facebook/jest/blob/master/CHANGELOG.md#2700. Thanks @mmkal!

@quantizor
Copy link

Did this get reverted at some point? Updating inline snapshots is clobbering the test file again... jest@27.5.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ranges Issues about formatting/ignoring/etc segments of files help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue!
Projects
None yet
Development

No branches or pull requests

13 participants