feat: make prettier optional for inline snapshots

[mmkal]

Summary

Inline snapshots are awesome, but they use prettier to format the test file whenever they're added or updated. This is bad for two reasons:

1. It affects code outside of the snapshots.
2. It creates a dependency on prettier.

In projects that use prettier, both of these are fine. But in projects that use other style tools, this can mean that prettier will fight with those other tools, and their configurations need to be kept in sync.

This PR addresses (1), but not (2). I would expect it could address (2) fairly easily too - prettier is now effectively only being used as a parser-finder, which provides a consistent AST to be walked. It's very possible there are more direct ways to do that, but I'm not familiar with prettier or babel, so I've left that for now. Any tips welcome - otherwise maybe it could be done as a follow up.

This PR uses prettier when an up-to-date version is found; otherwise falls back to using Babel 7 to walk the AST and locate the snapshots.

I opened an issue in prettier (prettier/prettier#5807) but the suggestion was to fix here instead. I tried using the the range API (as I think did @azz when he first implemented inline snapshots in #6380), but there were a lot of gotchas and this approach is closer to getting rid of the prettier dependency entirely.

The basic implementation is - walk the AST in the same way as before, but instead of modifying at and using prettier to output a 'fixed' version, mark each snapshot with the character offsets of where they should be inserted. Then, walk backwards through each snapshot, and slice up the source file, inserting the snapshot text. Walking backwards means the offsets aren't messed up by the insertions.

Tagging a few people who took part in #6380:
@azz @SimenB @cpojer

Closes #7744

Test plan

Added some tests to inline_snapshots.test.js:

• one features hideous formatting outside of a snapshot assertion, and makes sure that the hideous formatting is preserved.
• another adds a test case for creating a new snapshot when a property matcher exists. This was just a missing test case and I wanted to make sure I didn't break this behaviour.

Command run:
npm run jest packages/jest-snapshot/src/__tests__/inline_snapshots.test.js
Output:

 PASS  packages/jest-snapshot/src/__tests__/inline_snapshots.test.js
  √ saveInlineSnapshots() replaces empty function call with a template literal (171ms)
  √ saveInlineSnapshots() leaves formatting outside of snapshots alone (9ms)
  √ saveInlineSnapshots() replaces existing template literal - babylon parser (3ms)
  √ saveInlineSnapshots() replaces existing template literal - flow parser (2ms)
  √ saveInlineSnapshots() replaces existing template literal - typescript parser (2ms)
  √ saveInlineSnapshots() replaces existing template literal with property matchers (3ms)
  √ saveInlineSnapshots() creates template literal with property matchers (3ms)
  √ saveInlineSnapshots() throws if frame does not match (28ms)
  √ saveInlineSnapshots() throws if multiple calls to to the same location (6ms)
  √ saveInlineSnapshots() uses escaped backticks (3ms)
  √ saveInlineSnapshots() works with non-literals in expect call (5ms)

Test Suites: 1 passed, 1 total
Tests:       11 passed, 11 total
Snapshots:   0 total
Time:        2.374s, estimated 5s

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[SimenB]

I like this approach! However, I think for this to be tenable we need to use something that's not prettier to walk the AST (I realize you talked about this in the OP). The approach of just walking backwards and modifying makes sense to me.

You have POC in the title - what do you think is missing? The only thing I can think of is to be able to still use prettier - otherwise I'll probably have to format my file after Jest has done it's thing. I really like that it's properly formatted automatically.

(also, please update the changelog (and docs))

[jeysal]

Regarding the optional usage of Prettier:
Maybe we can make it work both for Prettier users and non-users without requiring config by running Prettier on the old test file (without writing) to check if it changes anything. If it doesn't, we run Prettier after inserting the snapshot.
It's a bit expensive for Prettier users, but it would happen only on snapshot update, so it should be fine.

[mmkal]

@SimenB the main reason for POC is I wanted to see what the feedback was on using prettier to walk the AST without using it for formatting. Given there already is a dependency on prettier, my hope was that this at least solves the problem of unwanted formatting as a side-effect, so it may be good enough as a first step.

Since babel now supports typescript, maybe @babel/parser would be good enough.

[azz]

This is a good optimisation of the current implementation of inline snapshots, but might be missing the broader context and reason I used Prettier in the first place.

The end-game for inline snapshots is to be able to automatically create assertions that look like regular tests. For example, saving this file:

test('inline snapshot', () => {
  const obj = {
    foo: 'bar',
    fn: () => {},
  };
  expect(obj).toMatchInlineSnapshot();
});

Would auto-generate this:

test('inline snapshot', () => {
  const obj = {
    foo: 'bar',
    fn: () => {},
  };
  expect(obj).toMatchInlineSnapshot({
    foo: 'bar',
    fn: expect.any(Function),
  });
});

Using strings for the snapshots were just a way of getting the feature of the ground (MVP if you will), and creating the framework for implementing more powerful, JavaScript/expect snapshots, which come with better diagnostics, edit-ability, static analysis, etc.

So as much as this PR improves the way the feature is currently implemented, it may be pre-maturely optimising.

@cpojer do you agree?

[mmkal]

@azz that does sound good, but out of scope for this PR - at least I wouldn't be able to make that big a change.

@SimenB @jeysal in the end I went with making prettier optional. If there's no prettier, or if the version is too low, it uses @babel/parser to try to parse the AST, then string manipulation. If a new enough prettier version is found, the behaviour is the same as before.

IMHO, long-term prettier should just be dropped. But I'm biased since I don't use it, hence this PR. I guess if people care enough about formatting the file after insertion, it's worth maintaining both code paths.

[azz]

My point was Prettier provides the ability to parse, manipulate and re-print the AST without doing string manipulation. This will be required when generating more complex ASTs than just a template string literal. You could do this with @babel/core, @babel/parser and @babel/generator, but generator has never really been optimized for humans.

[mmkal]

@azz I see - if anything I think that's all the more reason to put this in now, before the coupling with prettier is increased. I think you should do that with @babel/generator, not prettier. That's exactly the sort of thing @babel/generator is for. So I don't really see this change as an optimisation. Side note - babel now has support for typescript, but only in version 7, which hadn't been released when inline snapshots were first added. There's no reason I'm aware of that it wouldn't be possible to do everything we need with babel now.

I got the impression from the original PR that using prettier was a stopgap to get the feature out:
#6380 (comment)
#6380 (comment)

[jeysal]

What a monster 😅

[jeysal]

So after benchmarking this on 64 test files with a noop test case each, it appears to be between 1.05x and 1.1x as fast as master due to the reduced overhead of loading prettier once per worker instead of once per test file.
I've also tried loading some other things in the dependency graph of jest-circus from outside, but e.g. all of {pretty-format,jest-diff,chalk} (some of the biggest ones) only gets us another 1.05x to 1.1x on top, and I'd expect it to cause all sorts of realm-related problems so it's nowhere near a 'free' perf gain either.
But still, of course we needed this anyway because without it loading prettier so many times OOMed CI lol, plus we now have the possibility to easily load big dependencies from inside the test runner if we need more in the future. I'm also still looking into some other areas where it could be applied for perf gains, we'll see what comes out of that.

[SimenB]

I pushed one cleanup commit to reduce the diff. @jeysal PTAL and merge if you're happy with it 👍 (with a more desriptive commit message than "uglier")

[jeysal]

🎉

[SimenB]

took almost 2 years 😅 Thanks @mmkal!

[github-actions]

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.