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

[Bug]: Prettier v3 doesn't work with jest-snapshots #14305

Closed
TAGraves opened this issue Jul 6, 2023 · 25 comments · Fixed by #14566
Closed

[Bug]: Prettier v3 doesn't work with jest-snapshots #14305

TAGraves opened this issue Jul 6, 2023 · 25 comments · Fixed by #14566

Comments

@TAGraves
Copy link

TAGraves commented Jul 6, 2023

Version

29.6.1

Steps to reproduce

  1. Install prettier@3.0.0, which was just released yesterday
  2. Write a test that would generate an inline snapshot, like:
test('snapshot', () => {
  expect('abc').toMatchInlineSnapshot();
});

Expected behavior

The inline snapshot is updated

Actual behavior

Jest fails with

  ● Test suite failed to run

    TypeError: prettier.resolveConfig.sync is not a function

      at runPrettier (node_modules/jest-snapshot/build/InlineSnapshots.js:308:30)

Additional context

Prettier just released v3, which removed this line that added the sync function to resolveConfig. It looks like eslint-plugin-prettier ran into a similar problem. based on this issue.

Environment

System:
    OS: macOS 13.4.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
    npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm
    pnpm: 8.5.1 - ~/Library/pnpm/pnpm
  npmPackages:
    jest: ^29.6.0 => 29.6.1
@mrazauskas
Copy link
Contributor

mrazauskas commented Jul 8, 2023

Confirmed: if Prettier v3 is installed, Jest fails to update or create inline snapshots. Existing inline snapshots work fine if they are passing.

Seems like this is similar to what eslint-plugin-prettier was facing – TypeError: prettier.resolveConfig.sync is not a function. See prettier/eslint-plugin-prettier#562

@mrazauskas
Copy link
Contributor

Seems like it is possible to add prettierPath: null to Jest config. Formatting can be done using Prettier outside of Jest run. That can be a workaround until solution will be found.

@SimenB
Copy link
Member

SimenB commented Jul 10, 2023

I doubt we'll ever support Prettier v3 as we need it to work synchronously.

However, we might be able to add support for @prettier/sync.

@SimenB
Copy link
Member

SimenB commented Jul 10, 2023

That module is currently broken (prettier/prettier-synchronized#5), but I think it should work after that is released. We'll then have to look at the version of prettier we load, and decide which API to use

@mrazauskas
Copy link
Contributor

@prettier/sync is hardcoding the path to prettier, so it will not work with prettierPath option. I was thinking, what if we would vendor it and adjust to the needs of Jest.

@fisker
Copy link
Contributor

fisker commented Jul 10, 2023

Maybe @prettier/sync shoud expost a factory to accept prettierPath instead?

@mrazauskas
Copy link
Contributor

That would great, of course.

@SimenB
Copy link
Member

SimenB commented Jul 10, 2023

Right, good point.

Passing a path for prettier sounds reasonable 👍

@SimenB
Copy link
Member

SimenB commented Jul 10, 2023

With the release of that, we should be able to integrate here.

@SimenB
Copy link
Member

SimenB commented Jul 27, 2023

FWIW, added an explicit error in https://github.com/jestjs/jest/releases/tag/v29.6.2, and updated the docs with workarounds

@ctsstc
Copy link

ctsstc commented Aug 8, 2023

For anyone that lands here for a temporary fix, this works for now:

  1. Manually add "prettier-2": "npm:prettier@^2", to devDependencies in your package.json
  2. Run npm install
  3. Add prettierPath to your jest configuration
    • ie: "prettierPath": "<rootDir>/node_modules/prettier-2/index.js"

@ecraig12345
Copy link

ecraig12345 commented Aug 8, 2023

The suggestion above is a good idea, but it may have some problematic side effects: if prettier and prettier-2 are installed in devDependencies of the same package, some package managers (definitely yarn 1, likely others too) will nondeterministically choose which prettier binary (from v2 or v3) to link under node_modules/.bin. So it may appear to work at first, but then cause errors or mysterious inconsistencies in CI or on someone else's computer.

I wish I could think of a better way around this, but the best I can come up with is a postinstall script to fix the link if needed:

const path = require('path');
const fs = require('fs');

// TODO: Update this to point to your node_modules path relative to the script location
const nodeModulesPath = path.resolve(__dirname, 'node_modules');
const prettierBinPath = path.join(nodeModulesPath, '.bin/prettier');

if (fs.realpathSync(prettierBinPath).includes('prettier-2')) {
  fs.rmSync(prettierBinPath);
  const packageJson = JSON.parse(fs.readFileSync(path.join(nodeModulesPath, 'prettier/package.json'), 'utf-8'));
  const binFile =
    typeof packageJson.bin === 'string' ? packageJson.bin : packageJson.bin.prettier;
  const realBinPath = path.join(nodeModulesPath, 'prettier', binFile);
  fs.symlinkSync(realBinPath, prettierBinPath);
}

@louisgv
Copy link

louisgv commented Aug 20, 2023

Easiest would be to make toMatchInlineSnapshot async. But that's horribly breaking

Maybe making a new toMatchInlineSnapshotAsync?

DaveTryon added a commit to microsoft/accessibility-insights-action that referenced this issue Sep 22, 2023
#### Details

Due to jestjs/jest#14305, our inline snapshots
are causing problems when using `yarn test -u`. We discussed this
internally and decided that the simplest approach was to convert the
inline snapshots to traditional snapshots. For most cases, this was just
removing "Inline" from the method name and removing the string that
provided the snapshot. A small number of test cases were checking
multiple inline snapshots in the same test, so the approach there was to
build an array of the things that were previously checked individually,
then having a single snapshot for the test case.

I ran `yarn test -u` and `yarn format:fix` after making all of the
changes, just to make sure that we're ready for future changes.

It will probably be easiest to review this one commit at a time--each of
the 32 commits represents the conversion of a single test case.

##### Motivation

<!-- This can be as simple as "addresses issue #123" -->

##### Context

<!-- Are there any parts that you've intentionally left out-of-scope for
a later PR to handle? -->

<!-- Were there any alternative approaches you considered? What
tradeoffs did you consider? -->

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [n/a] Addresses an existing issue:
- [x] Added relevant unit test for your changes. (`yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] Ran precheckin (`yarn precheckin`)
DaveTryon added a commit to microsoft/accessibility-insights-web that referenced this issue Sep 22, 2023
#### Details

Due to jestjs/jest#14305, our inline snapshots
are causing problems when using `yarn test -u`. We discussed this
internally and decided that the simplest approach was to convert the
inline snapshots to traditional snapshots. For most cases, this was just
removing "Inline" from the method name and removing the string that
provided the snapshot. A small number of test cases were checking
multiple inline snapshots in the same test, so the approach there was to
build an array of the things that were previously checked individually,
then having a single snapshot for the test case.

I ran `yarn test -u` and `yarn format:fix` after making all of the
changes, just to make sure that we're ready for future changes.

It will probably be easiest to review this one commit at a time--each of
the 32 commits represents the conversion of a single test case.

##### Motivation

<!-- This can be as simple as "addresses issue #123" -->

##### Context

<!-- Are there any parts that you've intentionally left out-of-scope for
a later PR to handle? -->

<!-- Were there any alternative approaches you considered? What
tradeoffs did you consider? -->

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [n/a] Addresses an existing issue
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS
@OmarJaroudi
Copy link

@SimenB Just curious about when/how the release process works to publish a new version with the changes merged in #14566, thanks!

@SimenB
Copy link
Member

SimenB commented Oct 3, 2023

It'll be in Jest 30. Not sure when there'll be a release, but within the next few weeks, hopefully 🙂

@benjamin-guibert
Copy link

Thanks for the good work! 💪🏻

nikomakela added a commit to City-of-Helsinki/kukkuu-admin that referenced this issue Oct 11, 2023
nikomakela added a commit to City-of-Helsinki/kukkuu-admin that referenced this issue Oct 12, 2023
kfcampbell added a commit to octokit/oauth-methods.js that referenced this issue Oct 19, 2023
The manual reason is because Prettier 3 isn't supported by Jest (yet).
See [Jest
docs](https://jestjs.io/docs/configuration/#prettierpath-string) and
[this Jest tracking issue](jestjs/jest#14305).
renovate bot added a commit to octokit/oauth-methods.js that referenced this issue Oct 19, 2023
* build(deps): lock file maintenance

* Use @gr2m's fetch-mock

* Update octokit/types version

* Manually update snapshots

The manual reason is because Prettier 3 isn't supported by Jest (yet).
See [Jest
docs](https://jestjs.io/docs/configuration/#prettierpath-string) and
[this Jest tracking issue](jestjs/jest#14305).

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
nikomakela added a commit to City-of-Helsinki/kukkuu-admin that referenced this issue Oct 20, 2023
nikomakela added a commit to City-of-Helsinki/kukkuu-admin that referenced this issue Oct 24, 2023
@Dzieni
Copy link

Dzieni commented Oct 30, 2023

@SimenB Do you know already when Jest v30 will be released? Is there some nightly release that I can install right now and use Prettier v3?

@SimenB
Copy link
Member

SimenB commented Oct 30, 2023

I don't have a firm date in mind, but I can probably do a prerelease.

Nightlies would be awesome (or CI publishing in general), but we don't have that at the moment, unfortunately.

@SimenB
Copy link
Member

SimenB commented Oct 30, 2023

https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.1

lawvs added a commit to lawvs/poi-plugin-quest-2 that referenced this issue Nov 6, 2023
barona-mika-vilpas added a commit to mikavilpas/parjs that referenced this issue Nov 21, 2023
The version is needed because currently jest snapshots cannot be used if prettier 3 is used.
It has been fixed but is not released in a non-alpha version.

jestjs/jest#14305
tonyxiao added a commit to useVenice/venice that referenced this issue Nov 25, 2023
Copy link

This issue 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 Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.