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

Storybook - forward Storybook's native parameters in CLI #7531

Closed
danr-za opened this issue Oct 27, 2021 · 26 comments · Fixed by #12238
Closed

Storybook - forward Storybook's native parameters in CLI #7531

danr-za opened this issue Oct 27, 2021 · 26 comments · Fixed by #12238
Assignees
Labels
outdated scope: storybook Issues related to Storybook support in Nx type: feature

Comments

@danr-za
Copy link
Contributor

danr-za commented Oct 27, 2021

Description

Storybook supports various parameters such as output-dir (which is for example needed by Chromatic), log-level, opening the browser, ssl and others.
It would be helpful to simply forward them when using nx storybook cli.

Motivation

Native Storybook support

@mandarini mandarini added the scope: storybook Issues related to Storybook support in Nx label Oct 27, 2021
@cakeinpanic
Copy link

Hi, are there any updates on it?

@danr-za
Copy link
Contributor Author

danr-za commented Jan 11, 2022

any feedback will be appreciated 🙃 @mandarini

@mandarini
Copy link
Member

Hi there! No news on that yet. It's a feature request, and we're prioritizing bugs at the moment. I will let you know. However, in the meantime, if any of you would like to give this a shot, let me know, and I can help you! :)

@danr-za
Copy link
Contributor Author

danr-za commented Jan 13, 2022

Sure thing.
I'll try to submit a PR by the weekend or so.
Thanks!

@mandarini
Copy link
Member

Hi there, on second thoughts, I think that the native parameter forwarding should work as it is now. They may not appear in our schema, but I think they are forwarded. @danr-za can you give it a try and let me know? I mean, just pass the parameters as you would normally, and observe if they work or not..

@danr-za
Copy link
Contributor Author

danr-za commented Mar 10, 2022

@mandarini I tried with:

  • --open (even though I am not sure it exists, because storybook opens the browser by default, so they have --no-open)
  • --docs (appears as docsMode on nx schema, but wasn't forwarded)
  • --ci
  • --webpack-stats-json

@mandarini
Copy link
Member

mandarini commented Mar 17, 2022

Hi there @danr-za ! :) I am in the process of changing our storybook executor for Angular, from our custom one to the native Storybook one. This should mean that it also supports any other params. Do you want to give this a shot? Here's the PR: #9332

@mandarini
Copy link
Member

mandarini commented Mar 18, 2022

@danr-za @cakeinpanic Oh, also, btw, did you try adding these options in the project.json, under the storybook target options? Anything in that options object gets passed down, as a Storybook CLI option.

@mandarini mandarini self-assigned this Mar 18, 2022
@danr-za
Copy link
Contributor Author

danr-za commented Mar 22, 2022

@mandarini nope, just in the cli (but I guess both should be supported)

@mandarini
Copy link
Member

Hi @danr-za ! I checked today, and I could not reproduce. I mean, I tried passing a number of parameters (either them being in our schema or not) and they all worked, except the --open one (because we hard-code ci: true but that's another story). I mean, can you send me a repo where you tried adding params through the CLI and they did not work? I mean, repo + commands!

@danr-za
Copy link
Contributor Author

danr-za commented Apr 13, 2022

ye, hope I can take it this week.

@mandarini
Copy link
Member

mandarini commented Apr 13, 2022

Perfect, thanks @danr-za ! btw, I debugged this during my last stream, if you care to see the whole debugging process.

@mandarini
Copy link
Member

I'm linking this PR here since it's related (only for the --open flag).

@mandarini
Copy link
Member

I am closing this, since I am 99.9% sure that it works as expected with all the latest changes. Please feel free to reopen, after you test with latest version of Nx!

Thank you all!

@janeklb
Copy link
Contributor

janeklb commented Sep 16, 2022

Hi @mandarini

I don't believe this is working as expected -- unless, of course, we expect different things :)

For example, if you have the following script in your package.json

{
  "scripts": {
    "build-storybook": "nx build-storybook my-app",
  }
}

and you run

npm run build-storybook -- --webpack-stats-json

The @nrwl/storybook:build executor will not forward the cli arg (--webpack-stats-json) to the storybook builder.

We are able to work around it by adding the necessary config (webpackStatsJson: true) to the executor's options in project.json (what you mentioned here); however, it introduces some very brittle coupling between our project.json and the process that invokes the build-storybook command: chromaui/action (chromatic's Github Action).

The chromaui/action action runs the following command

npm run --silent build-storybook -- --output-dir /tmp/chromatic--1761-OSRHsIVpk1KR --webpack-stats-json /tmp/chromatic--1761-OSRHsIVpk1KR

but then spits out a warning saying that the storybook did not get built in the expected /tmp/chromatic--1761-OSRHsIVpk1KR directory (it is instead built in the directory specified the executor's options inproject.json etc.)

I believe this qualifies as unexpected behaviour - would you agree?

@mandarini
Copy link
Member

Hey @janeklb sorry just seeing this. I promise to look into it this week!

@jaknas
Copy link

jaknas commented Sep 23, 2022

@danr-za @cakeinpanic Oh, also, btw, did you try adding these options in the project.json, under the storybook target options? Anything in that options object gets passed down, as a Storybook CLI option.

Could NX make it visible in the documentation for the executor? This behavior is not obvious.

@mandarini
Copy link
Member

Sure, I'll make sure to update it next week. Thanks for the feedback!

@mandarini
Copy link
Member

@janeklb I pushed a fix (we're not overriding output-dir any more). I think that should fix the issues you are having, and it makes sense, too. Thanks for pointing it out.

I will eventually fix the docs with some examples.

mandarini added a commit to mandarini/nx that referenced this issue Sep 26, 2022
mandarini added a commit to mandarini/nx that referenced this issue Sep 26, 2022
mandarini added a commit to mandarini/nx that referenced this issue Sep 26, 2022
mandarini added a commit to mandarini/nx that referenced this issue Sep 27, 2022
mandarini added a commit that referenced this issue Sep 27, 2022
@janeklb
Copy link
Contributor

janeklb commented Sep 28, 2022

Thanks @mandarini!

The fix (#12238) doesn't seem to enable all CLI arguments to be passed to the storybook builder. It does solve my specific problem; however, I'm now just curious if this is intentional or simply just not something that's been worked out?

@mandarini
Copy link
Member

Oh @janeklb you see these two arguments are the only ones we override. All the rest we are passing them as they are!

@mandarini
Copy link
Member

@janeklb if you look at the same file, a few lines above:

function storybookOptionMapper(
  builderOptions: StorybookBuilderOptions,
  frameworkOptions: any,
  context: ExecutorContext
) {
  const storybookOptions = {
    ...builderOptions,
    ...resolveCommonStorybookOptionMapper(
      builderOptions,
      frameworkOptions,
      context
    ),
    mode: builderOptions?.['mode'] ?? 'static',
    outputDir:
      (builderOptions?.['outputDir'] || builderOptions?.['output-dir']) ??
      builderOptions.outputPath,
  };

  return storybookOptions;
}

take note at:

  const storybookOptions = {
    ...builderOptions,

We are passing all the builder options as they are. No changes.

However, below, we are were overriding outputDir with outputPath and mode with static. This is what I fixed. First check if there is outputDir or mode already passed as options, if they are not, write them, if they are there, don't touch them!

@janeklb
Copy link
Contributor

janeklb commented Oct 1, 2022

Thank you for taking the time to explain - much appreciated :)

@mandarini
Copy link
Member

:D

@sebpalluel
Copy link

Hi @mandarini

I don't believe this is working as expected -- unless, of course, we expect different things :)

For example, if you have the following script in your package.json

{
  "scripts": {
    "build-storybook": "nx build-storybook my-app",
  }
}

and you run

npm run build-storybook -- --webpack-stats-json

The @nrwl/storybook:build executor will not forward the cli arg (--webpack-stats-json) to the storybook builder.

We are able to work around it by adding the necessary config (webpackStatsJson: true) to the executor's options in project.json (what you mentioned here); however, it introduces some very brittle coupling between our project.json and the process that invokes the build-storybook command: chromaui/action (chromatic's Github Action).

The chromaui/action action runs the following command

npm run --silent build-storybook -- --output-dir /tmp/chromatic--1761-OSRHsIVpk1KR --webpack-stats-json /tmp/chromatic--1761-OSRHsIVpk1KR

but then spits out a warning saying that the storybook did not get built in the expected /tmp/chromatic--1761-OSRHsIVpk1KR directory (it is instead built in the directory specified the executor's options inproject.json etc.)

I believe this qualifies as unexpected behaviour - would you agree?

I can confirm the only way I've found to make the TurboSnap feature to work along with nx is by directly adding the webpackStatsJson option here:
https://github.com/sebpalluel/web3-monorepo/blob/bf1cd644fe46c1be5e4f1cbd97ab4bb67a090a2d/apps/web/project.json#L120-L134

It seems the option --webpack-stats-json is always bypassed otherwise.

Here is my github CI workflow for Chromatic by the way:

https://github.com/sebpalluel/web3-monorepo/blob/main/.github/workflows/chromatic-web.yml

And the script to build only the storybook of my web app: "build-storybook:web": "nx run web:build-storybook",

That way I can have multiple instances of storybook as described in the chromatic guideline for monorepo:
https://www.chromatic.com/docs/github-actions#run-chromatic-on-monorepos

@github-actions
Copy link

github-actions bot commented Apr 8, 2023

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: storybook Issues related to Storybook support in Nx type: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants