Inline snapshots are not written to js files containing JSX syntax when the file is located in a subfolder

[JimMalmborg]

💥 Regression Report

Inline snapshots are not written to JavaScript files containing JSX syntax when the file is located in a subfolder. It works fine if the file is at the root of the project. It also works fine for TypeScript files in both root and subfolders.

Last working version

Worked up to version: 26.6.3

Stopped working in version: 27.0.0

To Reproduce

Steps to reproduce the behavior:

1. Create a React dev environment using the following packages or clone my repo.

"@babel/core": "^7.15.0",
"@babel/preset-env": "^7.15.0",
"@babel/preset-react": "^7.14.5",
"babel-jest": "^27.0.6",
"jest": "^27.0.6",
"prettier": "^2.3.2",
"react-test-renderer": "^17.0.2"

2. Create a JavaScript test file in a subfolder e.g. ./src/inline-snapshot.spec.js

import React from 'react';
import TestRenderer from 'react-test-renderer';

it('should produce inline snapshot', () => {
  const result = TestRenderer.create(<span>test</span>).toJSON();
  expect(result).toMatchInlineSnapshot();
});

3. Run the test using jest

Expected behavior

The inline snapshot should be written to the test file.

Link to repl or repo (highly encouraged)

https://replit.com/@JimMalmborg/inline-snapshot-regression?v=1

https://github.com/JimMalmborg/inline-snapshot.git
Run yarn and yarn test

Note! Manually clear any existing snapshots from the test files before running the tests.

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: Windows 10 10.0.19043
    CPU: (16) x64 Intel(R) Core(TM) i7-10875H CPU @ 2.30GHz
  Binaries:
    Node: 15.4.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 7.0.15 - C:\Program Files\nodejs\npm.CMD
  npmPackages:
    jest: ^27.0.6 => 27.0.6

[sigveio]

Hey @JimMalmborg! Thanks for providing a detailed description along with repo links.

All 3 tests successfully produce inline snapshots in a properly configured environment here, so I do not believe this is a bug in Jest. For troubleshooting help, please see the tips I gave another user reporting a similar problem in #11730

[JimMalmborg]

Hey @jimbergstrom! Thanks for providing a detailed description along with repo links.

All 3 tests successfully produce inline snapshots in a properly configured environment here, so I do not believe this is a bug in Jest. For troubleshooting help, please see the tips I gave another user reporting a similar problem in #11730

Yes, that might be the case. But I did try to set up the replit env according to the jest getting started documentation + babel from scratch, super basic, and almost zero configuration. Also, changing the version back to 26.6.3 solved the issue, that's is why I assumed it was a bug.

Do you have a repo with a properly configured environment I can check?

[JimMalmborg]

Hm, I did get my examples to work by just renaming babel.config.js to .babelrc.js

Hey @jimbergstrom! Thanks for providing a detailed description along with repo links.
All 3 tests successfully produce inline snapshots in a properly configured environment here, so I do not believe this is a bug in Jest. For troubleshooting help, please see the tips I gave another user reporting a similar problem in #11730

Yes, that might be the case. But I did try to set up the replit env according to the jest getting started documentation + babel from scratch, super basic, and almost zero configuration. Also, changing the version back to 26.6.3 solved the issue, that's is why I assumed it was a bug.

Do you have a repo with a properly configured environment I can check?

Update: I did get my tests to pass by just renaming my babel.config.js to .babelrc.js in my test repo 🎉 So maybe a misconfiguration of babel configs in our large product repo. But I still think it's a bit weird that this stopped working with v27, and only a problem for inline snapshots in JS files located in a subdirectory 🤷‍♂️

Anyway, feel free to close the issue if you still think it's not a bug.

[sigveio]

(...) changing the version back to 26.6.3 solved the issue, that's is why I assumed it was a bug.

Yeah don't worry, it's not unusual to make that assumption. But what many seem to not consider well enough is that packages in the NPM ecosystem tend to follow semantic versioning, and a bump in major version (e.g. 26.x to 27.x) signals potential breaking changes. This can for example be directly in the package you are using and/or related to dependencies.

Update: I did get my tests to pass by just renaming my babel.config.js to .babelrc.js in my test repo 🎉

Good job! 🙌

Anyway, feel free to close the issue if you still think it's not a bug.

Hmm...I might have to eat my words when it comes to "properly configured environment". When you said that .babelrc.js worked and not babel.config.js it peaked my curiosity to take a closer look.

In parts of the Babel docs these are mentioned in a way that makes them sound like they are treated equally. And the Jest docs do tell you to use babel.config.js for configuring Babel. So I thought.... at the very least there could be a documentation issue in one of the projects.

You are also right in that the error appears to happen even when the test itself is correctly transformed.

Which, upon second thought, I agree seems odd. It might suggest a discrepancy in config loading.

So I took a look at how Jest calls Babel to parse into AST for building the inline snapshot:

https://github.com/facebook/jest/blob/fdc74af37235354e077edeeee8aa2d1a4a863032/packages/jest-snapshot/src/InlineSnapshots.ts#L96-L101

Notice how the root is set to the directory of the sourcefile.

When Babel then attempts to build a config chain, it first tries to resolve what it should treat as the root directory of the project. This is determined by an option called rootMode, and the default is to attempt to load a babel.config (the 'project-wide configuration') with one of the allowed extensions (.json, .js, .cjs, .mjs) and root as the dirname.

Now I do suddenly understand what you meant with things working fine if the tests are 'not in a subfolder' - as in; at the root level of the project. That makes sense, because if the test and babel.config.js are at the same level in the directory structure, root and the directory of babel.config.js are the same. If you copy babel.config.js into the src folder, you'd see that it works as well.

And the reason it works with the .babelrc.js is that this is treated as a file-relative configuration, which has an entirely different loading mechanism that essentially walks up the directory structure until it hits the first package.json... and then loads any .babelrc that lives next to it. Meaning that in for example a Monorepo, your workaround wouldn't be effective unless you did it at the package level.

So this is starting to look like a potential bug to me.

[sigveio]

@mmkal - could I pick your brain on this one? The code in question was introduced in (the rather impressive 😅 ) PR #7792 that you worked on. Was there a reason (that you can remember) behind setting root to the directory of the test generating the inline snapshot?

If I delete the line which sets theroot from InlineSnapshots.ts and peak at the config Babel is running with, it seems to default to the correct root path of the project and behave as expected. However... the corresponding test fails.

https://github.com/facebook/jest/blob/fdc74af37235354e077edeeee8aa2d1a4a863032/packages/jest-snapshot/src/__tests__/InlineSnapshots.test.ts#L184-L192

Looking at this, I wonder if perhaps the test has a flaw when you consider how Babel looks for a .babelrc?

I.e. unless a dummy package.json is also generated next to it in the tmp folder... it probably can't work without root set to the same directory like it is now. But that essentially seem to break loading of project-wide Babel configuration for inline snapshots. Especially since it doesn't appear to be trivial to override for example rootMode in this context either.

[mmkal]

@sigveio I'm afraid I might not be much help here. I'm not very familiar with babel (I learned a lot of what I know about it writing that PR - and that was three years ago!). However I did restore the deleted branch so I could find the blame for individual commits on that branch: https://github.com/mmkal/jest/blame/uglier-inline-snapshots/packages/jest-snapshot/src/InlineSnapshots.ts#L93

From the blame, it looks like @jeysal committed the line in question so may be able to comment on whether it's safe to remove.

[jeysal]

Pretty sure I didn't write that, but I moved a lot of the code around in a hundred of rebases while the PR was open and I had taken over :P

Anyway, yeah it probably doesn't make sense the way it is set up, but I'm also not sure defaulting root to cwd would help? Maybe we need rootMode: 'upward-optional'?

As an alternative solution:
We only use Babel to parse, we don't run transforms. This means that any user-defined plugins don't matter anyway. The only plugins that we need are the syntax plugins (which aren't really plugins, but just enable a flag in the monolithic parser that tells it to allow a certain syntax). And of those syntax plugins, we could just enable all of them*, because the user might be using this syntax and we want to really make sure we parse successfully. Then configFile: false, babelrc: false, and done (hopefully) and much faster without the config resolution anyway.
(* as an exception, for TypeScript this is not a good idea and the existing code already only enabled JSX syntax for TypeScript when it's really a .tsx file; this is because in TypeScript enabling JSX actually doesn't just make more things parse successfully, but also makes some things no longer parse, because some TypeScript generic syntax conflicts with JSX)

I don't have time to work on Jest at the moment, but either of those could work if someone wants to try it 😅

[sigveio]

Thanks a lot for such a swift follow up, both of you!

@sigveio I'm afraid I might not be much help here. I'm not very familiar with babel (I learned 90% of what I know about it writing that PR - and that was three years ago!). However I did restore the deleted branch so I could find the blame for individual commits on that branch: https://github.com/mmkal/jest/blame/uglier-inline-snapshots/packages/jest-snapshot/src/InlineSnapshots.ts#L93

From the blame, it looks like @jeysal committed the line in question so may be able to comment on whether it's safe to remove.

I feel you - I learn much even just from trawling through the code trying to understand issues like this. Being able to tap into such a wealth of collective skill and knowledge is one of the things I truly love about open source. 😌

And hehe, yeah, I thought it might be a bit of a stretch asking anyone to remember such a specific detail from so long ago... especially considering the size of the PR and how many people were involved. I got sweaty just from trying to read through it all. 😅

Anyway, yeah it probably doesn't make sense the way it is set up, but I'm also not sure defaulting root to cwd would help? Maybe we need rootMode: 'upward-optional'?

Perhaps - that was mainly to poke at it and see how the basic reproduction together with the test suite from #7792 would react to it.

As an alternative solution:
We only use Babel to parse, we don't run transforms. This means that any user-defined plugins don't matter anyway. The only plugins that we need are the syntax plugins (which aren't really plugins, but just enable a flag in the monolithic parser that tells it to allow a certain syntax). And of those syntax plugins, we could just enable all of them*, because the user might be using this syntax and we want to really make sure we parse successfully. Then configFile: false, babelrc: false, and done (hopefully) and much faster without the config resolution anyway.

That's an interesting idea! 🤔

(* as an exception, for TypeScript this is not a good idea and the existing code already only enabled JSX syntax for TypeScript when it's really a .tsx file; this is because in TypeScript enabling JSX actually doesn't just make more things parse successfully, but also makes some things no longer parse, because some TypeScript generic syntax conflicts with JSX)

The thing that comes to mind for me would be angle brackets being ambiguous in certain settings between Flow, TS, and ES.

With Flow, the Babel parser by default handles it as an ES-style comparison unless the source starts with a // @flow pragma. Meaning that if it has an impact on snapshot generation/comparison it could perhaps be solved with documentation.

With TS the legacy type assertion could be problematic, but the TS syntax parser supports forcibly enabling JSX parsing. Could we do that and expect people to use the much more compatible as operator instead?

[jeysal]

The file extension-based system we already have should continue to work good enough for TS. If Flow also has these conflicts (I wasn't aware of that then that could make things difficult and maybe the former option of trying to use the existing Babel configs makes more sense.

And hehe, yeah, I thought it might be a bit of a stretch asking anyone to remember such a specific detail from so long ago... especially considering the size of the PR and how many people were involved. I got sweaty just from trying to read through it all. sweat_smile

Haha, you probably didn't even see the preceding PRs I had to build and land first to enable #7792 to go in without causing a massive performance hit and out-of-memory errors on CI 😂
(I just typed that #7792 by heart by the way, I think I will always remember that number :P)

[sigveio]

The file extension-based system we already have should continue to work good enough for TS. If Flow also has these conflicts (I wasn't aware of that then that could make things difficult and maybe the former option of trying to use the existing Babel configs makes more sense.

Yeah rootMode: 'upward-optional' is likely the path of least resistance; I will do some testing and consider a PR

Haha, you probably didn't even see the preceding PRs I had to build and land first to enable #7792 to go in without causing a massive performance hit and out-of-memory errors on CI 😂
(I just typed that #7792 by heart by the way, I think I will always remember that number :P)

Haha, I can imagine! Funnily enough I had already stumbled across that in other contexts, and it (along with the related sandbox escape hatch) was interesting reading! Good stuff! 🙌

[leepowelldev]

Not wanting to derail this conversation, but I just came across this issue when attempting to write inline snapshots (external snapshots work fine) - however, I didn't have any babel config (using ts-jest), and until I wanted to do inline snapshots all was working as expected. I have now created a babel config with the setup discussed and it works - why does inline snapshotting require a babel config to work?

[marcinczenko]

Just tried inline snapshot inside a workspace of a monorepo and it looks like I hit this issue. Is there anyone working on it? I can stick to regular snapshots for a moment...