Storybook: 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 https://github.com/storybookjs/storybook/issues/10673 and https://github.com/storybookjs/storybook/issues/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: https://github.com/storybookjs/storybook/issues/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!