Fix Storybook build args

[josemasar]

Fixes: #4448

Background:

When running yarn redwood storybook --build, it threw an error because the argument --open was set to true as default. --build and --open cannot be called at the same time.

Proposed solution:

The proposed solution is to turn the default behaviour of the argument --open to false.

In addition, I´m deleting another line which was running the arg --ci (skip interactive prompts, don't open browser) when calling --build and generating the same issue. --ci is not a permitted option when building storybook.

See: Storybook Docs

Tags:

@thedavidprice
@callingmedic911

[thedavidprice]

cc @virtuoushub (looping you in in case you have time to help me think through the logic we need here to handle all cases of running storybook.)

@josemasar Thank you for starting the work on this! Greatful for the help.

What we're running into here is a history of adding features (and conditions) to this CLI Command that has now overly complicated the logic used to build the command and options that are run. So I'd like to consider re-architecting the way we "build" the command that's run (starting at L80 with execa(yarn ${build ? ...`).

There are three things we can do with the Redwood Storybook command, each with applicable options

1. Start Storybook server
   (see all options via yarn start-storybook --help)

   • run start-storybook
   • open in browser (default true)
   • set port (default 7910)
   • directory options are hard-coded

2. Build Storybook static site/assets
   (see all options via yarn build-storybook --help)

   • build-directory option (default web/public/storybook)
   • manager-cache option (default true)
   • directory for config is hard-coded

3. Smoke-test: Check Storybook in CI
   CI mode plus Smoke-test (skip prompts, don't open browser, exit after successful start)

   • runs the start-storybook command; exits gracefully if started successful, throws otherwise
   • passes --smoke-test and --ci options

So instead of trying to build logic to handle all of these at once, what if we try to build logic based on each case — separating them out? The goal is to rework the complexity we currently have because it's all done at once starting on L80, which makes it really hard to understand what options are required (along with defaults) for the type of run a dev is trying to achieve.

• yarn rw sb: case 1. above
• with --build: case 2. above
• with --smoke-test: case 3. above

Questions? Feedback?

Lastly, ideally we'd add a simple test for this command, which would parse given input and make sure the output is what's expected. That could be a follow-on PR if/as needed — I don't want to slow this down by doubling the scope (twice 😆).

[josemasar]

@thedavidprice, when I read the issue, I thought the idea was to declutter the defaults options, so that the commands start and build will not be stepping into each other.

What you explained it makes a lot of sense because it will align better to the Storybook API and an user with previous experience using Storybook will feel familiar with our CLI. Furthermore, any potential change from Storybook, it will be easier to update.

I start working on this!

[thedavidprice]

@josemasar Completely understood. Frankly, I didn't even think about this until I saw what you tried to do, which helped me understand that we have a higher-level problem here — the mashup of conditionals.

Why don't you try to archtect for one case and push that change for us to discuss? Up to you if you want to handle it all at once. I'm just happy to iterate step by step if it feels more efficient for you.

[josemasar]

@thedavidprice , I separated the logic in the last commit. I tested in gitpod and launching 'yarn rw sb' and 'yarn rw sb --build' are giving the expected results. On 'yarn rw sb --smoke-test I couldn´t appreciate if there is a difference from 'yarn rw sb' because gitpod is not launching storybook properly. I guess at some point I´ll need to configure WSL for Windows to be more eficient when contributing to Redwood.

Besides, I´ll need to change the managerCache option to e.g. noManagerCache because the user has no ability to shift this parameter to false and storybook is caching always except if the option --no-manager-cached is called. So it´s better to align with the original API.

[thedavidprice]

@josemasar this is looking good! Still a few things left to do but really close — see my inline comments.

I tested in gitpod and launching 'yarn rw sb' and 'yarn rw sb --build' are giving the expected results. On 'yarn rw sb --smoke-test I couldn´t appreciate if there is a difference from 'yarn rw sb' because gitpod is not launching storybook properly. I guess at some point I´ll need to configure WSL for Windows to be more eficient when contributing to Redwood.

^^ Hmm, this is strange and maybe something is getting out of sync with GitPod. Just a reminder, if you're using the GitPod container linked from the PR, it only does a one-time "link" with the packages code from the branch. You'll need to run yarn rwfw project:sync yourself.

I can (and will) test myself as well. And I can tell 95% from the code whether things are looking good, which they are so far.

Besides, I´ll need to change the managerCache option to e.g. noManagerCache because the user has no ability to shift this parameter to false and storybook is caching always except if the option --no-manager-cached is called. So it´s better to align with the original API.

Yeah, this one is kinda backwards logic. We do want to use the original behavior, which requires using the && to either add the --no-manager-cache option or not.

Make sense?

[josemasar]

@thedavidprice, thanks for the guidance. I made all the changes that you suggested in the inline comments. I had to add an eslint-disable as we are not using open with the new conditionals logic.

[thedavidprice]

So close! Check out my comment regarding the open var. We'll be able to get this merged this week for sure.

Thanks again.

[josemasar]

@thedavidprice, I included the open argument and deleted the eslint disable comment.

This PR has been a great way to introduce me into the redwood codebase. The first days I read most of the files for building the CLI commands to get ideas to resolve this issue and adhere to your coding style. That´s been a good learning!

I´d like to continue contributing, so if during your triage sessions, you think an item would be good for me, just text me!

[josemasar]

Running yarn rw sb:

Running yarn rw sb --build

Running yarn rw sb --smoke-test

[virtuoushub]

...

Running yarn rw sb --smoke-test

Does the smoke test catch "bad" stories? e.g.

+ import StoryRouter from 'storybook-react-router'
import BlogPost from './BlogPost'

export const generated = () => {
  return <BlogPost />
}

- export default { title: 'Components/BlogPost' }
+ export default { title: 'Components/BlogPost', decorators: [StoryRouter()] }

Source: #3515 (comment)

[thedavidprice]

@virtuoushub re: bad stories --> confirmed ✅

[thedavidprice]

Excellent work here @josemasar 🚀

I´d like to continue contributing, so if during your triage sessions, you think an item would be good for me, just text me!

I have one if you're up for it! Basically taking over this one #4445

I'll follow up on Discord