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

CSF stories does not work correctly with addon-storyshots multiSnapshotWithOptions option #7697

Closed
jrsearles opened this issue Aug 6, 2019 · 17 comments

Comments

@jrsearles
Copy link

Describe the bug
I am trying out the beta for 5.2 and have converted several stories over to CSF. What i've found is that when running storyshots, all of the stories in the CSF style end up using the generic snapshot path/file instead of creating the snapshots adjacent to the stories.

To Reproduce
Steps to reproduce the behavior:

  1. Run jest with the storyshots add on enabled and the "--ci" flag
  2. Tests will fail because the snapshots are not in the expected location so are being considered new

Expected behavior
The story should use the same location as before to store snapshots.

Code snippets

// story-snapshots.test.ts
import initStoryshots, { multiSnapshotWithOptions } from "@storybook/addon-storyshots";

initStoryshots({
  test: multiSnapshotWithOptions({})
});

System:
OS: Windows 10
CPU: (8) x64 Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
Binaries:
Node: 10.15.0 - C:\Program Files\nodejs\node.EXE
Yarn: 1.16.0 - ~\AppData\Roaming\npm\yarn.CMD
npm: 6.10.0 - C:\Program Files\nodejs\npm.CMD
Browsers:
Edge: 42.17134.1.0
npmPackages:
@storybook/addon-actions: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-contexts: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-docs: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-knobs: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-storyshots: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-storyshots-puppeteer: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addon-viewport: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/addons: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/preset-scss: ^1.0.2 => 1.0.2
@storybook/react: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/source-loader: 5.2.0-beta.23 => 5.2.0-beta.23
@storybook/theming: 5.2.0-beta.23 => 5.2.0-beta.23

Additional context
I did some tracing and the problem appears to be the the filename for the story is undefined when the snapshot is being validated. I've traced where the filename is being determined:

const fileName = m && m.id ? `${m.id}` : undefined;

For CSF stories m is true here instead of an object. I believe the source may be this line here:

const kind = clientApi.storiesOf(kindName, true);

@shilman
Copy link
Member

shilman commented Aug 7, 2019

@jrsearles Good catch cc @tmeasday

@tmeasday
Copy link
Member

tmeasday commented Aug 7, 2019

🤔 I think CSF opens up the potential for refactoring how storyshots works in a much cleaner way (match on *.stories.js, write a Jest loader to map CSF => a test file).

In the short term is there an easy fix to this problem? I don't think we actually know the file name any more -- we could add an optional module to the default "component" export and pass that into storiesOf if it is there (that would change HMR behaviour, so we'd have to test that)

@tmeasday
Copy link
Member

tmeasday commented Aug 7, 2019

Or maybe we do know the filename from the req?

@shilman
Copy link
Member

shilman commented Aug 7, 2019

I think we can get the filename from the req as a workaround.

@stale stale bot added the inactive label Aug 28, 2019
@shilman shilman removed the inactive label Aug 28, 2019
@jrsearles
Copy link
Author

I can contribute with some guidance. This bug will prevent us from switching to CSF format.

@storybookjs storybookjs deleted a comment from stale bot Aug 31, 2019
@shilman
Copy link
Member

shilman commented Aug 31, 2019

Related: #7330

@Hypnosphi Hypnosphi self-assigned this Sep 4, 2019
@Hypnosphi
Copy link
Member

get the filename from the req

Unfortunately, it's possible only if first argument to configure is req or Array<req>, not () => Array<exports>.

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 5, 2019

Workaround: add {fileName: __filename} parameters to your CSF stories:

export default {
  title: 'My Stories',
  parameters: {fileName: __filename}
}

UPD: Until #8000 gets merged and released, you can also copy injectFileName.js to your project and include it in your jest.config.js like that:

transform: {
  // should be above any other js transform like babel-jest
  '^.+\\.stories\\.js$': '<rootDir>/path/to/injectFileName',
}

This will add parameters.fileName automatically

@shilman
Copy link
Member

shilman commented Oct 5, 2019

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.10 containing PR #8000 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Oct 5, 2019
@shilman
Copy link
Member

shilman commented Oct 7, 2019

Great Caesar's ghost!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.2 containing PR #8000 that references this issue. Upgrade today to try it out!

@mazikwyry
Copy link

@shilman @Hypnosphi I updated to 5.2.3 added: '^.+\\.stories\\.jsx?$': '@storybook/addon-storyshots/injectFileName', to jest.config.js and this is what I get:

Module @storybook/addon-storyshots/injectFileName in the transform option was not found..

Looks like the file is not included in the @storybook/addon-storyshots package.

image

@shilman
Copy link
Member

shilman commented Oct 9, 2019

@mazikwyry Ugh, this file got filtered out of the publish. Sorry about that. I've fixed it here and will re-publish once it's merged: #8354

cc @ndelangen @Hypnosphi

@shilman
Copy link
Member

shilman commented Oct 10, 2019

@mazikwyry Can you try 5.3.0-alpha.17? I'l patch it over to 5.2.x at the end of the week.

@mazikwyry
Copy link

@shilman

I updated to 5.3.0-alpha.17 added: '^.+\.stories\.jsx?$': '@storybook/addon-storyshots/injectFileName', to jest.config.js and this is what I get:

image

It also doesn't work when I use a workaround "^.+\\.stories\\.js$": "./injectFileName", which works in 5.2.x.

@Hypnosphi
Copy link
Member

@mazikwyry do you also have "^.+\\.js$": "babel-jest" entry?

@mazikwyry
Copy link

Yes I do

  // jest.config.js
  ...
  transform: {
     "^.+\.stories\.jsx?$": "@storybook/addon-storyshots/injectFileName",
    "^.+\\.js$": "babel-jest",
  },
  ...
    "babel-jest": "^24.5.0",
    "jest": "^24.7.0",
    "jest-haste-map": "^24.7.1",
    "jest-resolve": "^24.7.1",

Maybe there is something that I should change additionally for 5.3?

@Hypnosphi
Copy link
Member

@mazikwyry Can you please create a GitHub repo with minimal reproduction of your issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants