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

Storyshots should allow config object as well as config directory location #3260

Closed
theneva opened this issue Mar 22, 2018 · 8 comments
Closed

Comments

@theneva
Copy link
Contributor

theneva commented Mar 22, 2018

Issue details

I want to use Storyshots to verify the output of a component using react-beautiful-dnd, which requires refs. Since react-test-renderer does not support refs, I'm using Enzyme's mount as a renderer (which was my basis for #3252 yesterday).

However, with close to 100 stories to snapshot, it takes pretty much forever to use mount for all stories (and ~100 snapshots result in a gigantic (~1MB) .js.snap file) in a project that's expected to grow quite quickly (it's a shared CSS and component library for all teams in our company to use and contribute to).

For this reason, I've written two different test files that call initStoryshots. The two tests need have equal config, except two things:

  1. Which renderer to use
  2. Which stories are loaded

I can specify which renderer to use (1) as a config option to initStoryshots, which is nice.

However, which stories to load must be specified in config.js within the config direcrory location specified to initStoryshots. That means each separate test (calling initStoryshots) requires me to have a separate directory in my project with quasi-duplicated configuration.

Nesting config locations allowed me to get this beast down to something I suppose is okay (rtr means react-test-renderer):

image

The inner directories' load-<renderer>-stories.js files export a function that (when called) requires all the stories related to <renderer>. These functions are imported by stories-<renderer>/config.js and passed to @storybook/react's configure(<here>, module).

The outer directory is the configuration location of Storybook itself, which sets up addons, custom Webpack configuration for Storysource, and so on. The outer config.js requires and calls each load-<renderer>-stories.js to add them with the proper wrapping and addons.

It still feels a bit convoluted, though, and requires me to do this:

image

It would be much better if I could simply pass a function that loads (requires) stories (and, if needed, some addons config for Storybook and custom Webpack config) to initStoryshots instead of passing it a config directory location.

I would also like to be able to split the Storyshots tests into much smaller units (grouped by functionality) to be able to run the tests in parallel, as ~100 tests already takes quite a while. That quickly turns into a mess, as I would need to add new config directories and files and require them for both Storybook itself and each Storyshots test file.

I suppose this issue could (and should?) be resolved by adding parallelism to the Storyshots runner itself, but as it stands, allowing inline configuration to initStoryshots instead of configuration directories would help with this, too.

Steps to reproduce

N/A: this is a feature request.

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/react 3.3.15
  • @storybook/addon-storyshots 3.3.15

Affected platforms

  • Latest storyshots (3.3.15)
@igor-dv
Copy link
Member

igor-dv commented Apr 3, 2018

What about passing a path to the config.js itself? In this case, you will be able to have the following structure:

.storybook
 -- config.js
 -- config.specific.js
 -- config.another.js
initStoryshots({
  configPath: '.storybook/config.specific.js',
});

@theneva
Copy link
Contributor Author

theneva commented Apr 3, 2018

@igor-dv thanks for the response! I suppose that would help a little bit, but it would still require me to add a new configuration file (with close-to-duplicated configuration) for each story suite.

An option to consolidate the configuration concept into the initStoryshots function would allow me to to define a single file exporting different functions for different suites, and still be able to load them nicely from both Storyshots and Storybook itself.

Would what I propose be particularly difficult or time consuming to implement, or is it a matter of wanting to keep the API clean? I suppose I could try to implement what I want to be able to do, unless you have any objections to the idea.

@igor-dv
Copy link
Member

igor-dv commented Apr 4, 2018

The next release (v4.0) will be breaking, so we can change things to make an API better. If you have some prototype in mind feel free to share (and PR)

@theneva
Copy link
Contributor Author

theneva commented Apr 4, 2018

I'll have a look at it when I get some time, thanks a lot :)

@stale
Copy link

stale bot commented Apr 25, 2018

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!

@stale stale bot added the inactive label Apr 25, 2018
@stale
Copy link

stale bot commented May 25, 2018

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

@igor-dv
Copy link
Member

igor-dv commented Jun 13, 2018

Will be able to do it with passing a config function

@igor-dv igor-dv closed this as completed Jun 23, 2018
@issue-sh issue-sh bot removed the merged label Jun 23, 2018
@Hypnosphi
Copy link
Member

Released as 4.0.0-alpha.10

This issue was closed.
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

3 participants