Storyshots provide absolute vs relative path when a story is in storiesOf or CSF format when setting up custom directory

[GasimGasimzada]

Describe the bug

This is related to #10673 and #9084

I have been trying to understand this issue for a while now and finally I think I found the culprit for one of my problems. We have the following structure for saving visual test snapshots relative to components:

src/initial.visual.test.ts
src/components/SomeComponent/stories/__image_snapshots__
src/components/AnotherComponent/stories/__image_snapshots__
...

In order to achieve that, we are passing getMatchOptions function storyshots-puppeteer package's imageSnapshot function. This function was very simple:

const getMatchOptions = ({ context: { fileName }}) => {
  const snapshotPath = path.join(path.dirname(fileName), '__image_snapshots__');
  return { customSnapshotsDir: snapshotPath };
}

This used to work fine with storiesOf format because the fileName that is provided for storiesOf stories are always in absolute format:

/app/src/components/SomeComponent/stories/SomeComponent.stories.tsx

However, with CSF stories, the path start relative to initial test suite:

components/SomeComponent/stories/SomeComponent.stories.tsx

This created the problem of stories being written to random location and it was hard to find because we are running our visual tests in a Docker container. Now that we know the reason, we have solved the issue by removing the absolute path ourselves and setting it up:

const snapshotPath = path.join(__dirname, path.dirname(fileName.replace(__dirname, '')), '__image_snapshots__');

To Reproduce
Steps to reproduce the behavior:

1. Create storiesOf and CSF stories
2. Set up a custom getMatchOptions function
3. Print fileName from context
4. Run tests

Expected behavior

The path should either be consistent for both story formats or documented somewhere; so that, we are aware of it.

System:

• Storybook version: 6.0.21

[shilman]

That's pretty tricky and thank you for tracking it down.

@jonniebigodes Unfortunately fixing the problem is probably a breaking change, so I think for now our best bet is to document this, maybe in the Storyshots README or in a section of the official docs that describes the StoryContext (which I don't think exists, but probably should) WDYT?

[shilman]

Related: #7109

[latviancoder]

We are using both APIs in parallel, so we ended up with the following:

path.join(
  fileName.startsWith('./') ? srcDir : '',
  fileName,
  '../__image_snapshots__',
);

@shilman what do you think, does this look reliable?

[shilman]

@latviancoder that's a pretty cool ... workaround is a nice way to say it 😄

[jonniebigodes]

@shilman like you've mentioned this is a breaking change, we could probably use the workaround supplied by @latviancoder in both the addon Readme and the workflows/snapshot page to expand visibility. @latviancoder as you've supplied (at the lack of a better word) a workaround, if you're willing create the pull request and i'll gladly review it and we could merge it. If not i can take care of it. I'll leave it up to you, just let me know.

Also currently there's no concrete documentation for the StoryContext and it would be on our best interest to give some context to the community on how it works, we have brief references about it through out the documentation, but nothing concrete. I would be more than thankful for your help on this one. I'm going to take a look where we could place it and i'll let you know.

[latviancoder]

In the workaround above the srcDir variable is specific to our project. I'm not sure I understand how this could be solved on the library level.

[tmeasday]

OK, I worked out the source of the problem here. The TLDR is that the issue stems from the babel-plugin-require-context-hook package not adding req.resolve to the exported require.context object, so we just use the relative path here:

https://github.com/storybookjs/storybook/blob/6218082f3671c18d89cb9e959681c2f72824c25a/lib/core/src/client/preview/loadCsf.ts#L56:L57

Slightly more involved explanation:

1. If you use the stories field of main.js or just use require.context in your preview.js, this problem applies. If you require stuff directly then not so much.

2. For storiesOf files, the user calls storiesOf(..., module) and passes the (webpack OR node/jest) module to client_api, which grabs module.id (which is an absolute path in webpack and node) as fileName. This is the thing that ultimately makes it to the getMatchOptions function.

3. For CSF files, we never get access to the file's module object, instead we calculate the fileName based on the require.context "module" object in loadCSF.

4. require.context doesn't exist in node/jest however, so we use the above babel plugin to polyfill it. However that plugin doesn't implement the resolve() function on context modules, which means we don't currently resolve the path in the same way.

So the fix here is to add the resolve() function to the __context object that plugin exports.

One other wrinkle: it's not exactly clear, but it looks like webpack doesn't actually return an absolute path for the filename from req.context() either -- so we should probably be using require.resolve() to turn what it outputs into an absolute path too.

I suspect there might be dragons here, I'm not sure if we should rush this through for 6.1 or not. If someone wants to have a go at sending a PR to the babel plugin adding the resolve function that'd help a lot!

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[ekh64]

I ran into this today using Storyshots with simple snapshot testing as well. Is there a workaround to continue to use CSF while also having the snapshots colocated within the component directories?

[GriffinSauce]

This appears to still be an issue. Are there any changes that could help to sidestep it?

[GasimGasimzada]

@GriffinSauce the suggested that I provided worked for our scenario really well for us but if you have the capacity, I would strongly suggest to migrate to CSF or MDX stories as it solves a lot of incompatibility issues with newer versions of Storybook.

[GriffinSauce]

Thanks @GasimGasimzada I was actually migrating to CSF and the problem appeared 😅

I managed to make it work with two actions:

• we had a separate SB config for Storyshots that was in the old config style (IMO this should just be deprecated and cause an error, it's so easy to migrate), so that needed to be updated to main, preview etc.
• had to add fileName: __filename to the parameters for each story

Not very happy about the last thing... we're not using image snapshots (regular text snapshots) so I couldn't find anything like getMatchOptions to adjust and fix things globally.

[GasimGasimzada]

Not very happy about the last thing... we're not using image snapshots (regular text snapshots) so I couldn't find anything like getMatchOptions to adjust and fix things globally.

@GriffinSauce This is a long shot but you might be able to derive a class from Stories2SnapsConverter and sanitize the filename to your needs. I found it from their docs reference and it is specifically mentioned that:

This parameter should be an instance of the Stories2SnapsConverter (or a derived from it)

[ndelangen]

The future of storyshots is the test-runner:
https://storybook.js.org/docs/react/writing-tests/test-runner#page-top

And use the play function for expectations:
https://storybook.js.org/docs/react/writing-stories/play-function#page-top

We will not be making any improvement / changes to storyshots.