Allow passing both buildScriptName and storybookBuildDir

[woutervanvliet]

Allows passing both --storybook-build-dir and --build-script-name

Solves #933

[ghengeveld]

Hi Wouter, thanks for working on a fix for this!

I'm okay with your proposed change to allow --storybook-build-dir whilst buildScriptName is set in your chromatic.config.json. However, the original reason we had this restriction was to avoid confusion over which one gets priority. I propose the following solution:

• Rather than checking singularOpts against the final (combined) options object, check it against configuration (JSON), optionsFromFlags (flags / GitHub Action args) and extraOptions (Node API args) individually. This would allow overriding configuration using a flag or API arg, while preventing any misconfiguration or bad combination of flags/args.
• Print a warning when this happens.

Are you comfortable / willing to make this change?

[woutervanvliet]

Thanks for the input - I've reluctantly updated my PR, because I'm not sure I entirely agree. So here are a few followup questions:

• Am I right to assume that the presence of the buildScriptName configuration, regardless of where it comes from, doesn't actually have any effect - until the chromatic cli decides that a build is necessary because no build dir was specified?
• Am I also right to assume that build script name, when not specified, defaults to "build-storybook", and as such always can be considered to be specified?

If both answers are yes, than I don't think assumption that combining buildScriptName and storybookBuildDir in configuration and/or command line arguments in any way should indicate a potential mistake, and I think my latest commit should be reverted.

And if my assumptions are not entirely correct, please check if my change reflects your intention.

[ghengeveld]

• Am I right to assume that the presence of the buildScriptName configuration, regardless of where it comes from, doesn't actually have any effect - until the chromatic cli decides that a build is necessary because no build dir was specified?

Almost. The buildScriptName is also used to infer configDir and staticDir options from its flags. This is relevant for proper functioning of TurboSnap, even if storybookBuildDir is also specified. In that case it won't build Storybook but it'll assume the prebuilt Storybook used the same buildScriptName flags.

• Am I also right to assume that build script name, when not specified, defaults to "build-storybook", and as such always can be considered to be specified?

Yes, although we'll still check if that script actually exists in your package.json.

If both answers are yes, than I don't think assumption that combining buildScriptName and storybookBuildDir in configuration and/or command line arguments in any way should indicate a potential mistake, and I think my latest commit should be reverted.

After giving this some more thought, I think that's valid. There is actually a valid reason to have both a buildScriptName and a storybookBuildDir (for configDir/staticDir inference), so combining these may not necessarily be a mistake.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 85478071	✅ ∅

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (8547807)	Report Missing	Report Missing	Report Missing
Head commit (366f809)	8445	6640	78.63%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#934)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[ghengeveld]

🚀 PR was released in v11.2.0 🚀

[woutervanvliet]

Thanks

[ghengeveld]

Thanks for helping out @woutervanvliet !