Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Tech/allow many args CH-789 #447

Merged
merged 17 commits into from
Dec 7, 2021
Merged

Tech/allow many args CH-789 #447

merged 17 commits into from
Dec 7, 2021

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen requested a review from zol November 9, 2021 15:44
@zol zol changed the title Tech/allow many args Tech/allow many args CH-789 Nov 9, 2021
@linear
Copy link

linear bot commented Nov 9, 2021

CH-789 chromatic-cli should accept multiples of the same argument (and pick the last one provided)

Hi,
so i change my storybook-build script to:
"build-storybook": "nx run kudos:build-storybook --webpack-stats-json storybook-static"

this is the command chromatic run (and write in the log):
nx run kudos:build-storybook --webpackStatsJson=storybook-static --webpackStatsJson=/tmp/chromatic--1923-jCKKqn91uLU5 --outputDir=/tmp/chromatic--1923-jCKKqn91uLU5

and this is the error i get:
The "path" argument must be of type string. Received an instance of Array
———————————————————————————————————————————————
> NX ERROR Running target "kudos:build-storybook" failed
Failed tasks:

- kudos:build-storybook
→ Command failed: npm run --silent build-storybook -- --output-dir /tmp/chromatic--1923-jCKKqn91uLU5 --webpack-stats-json /tmp/chromatic--1923-jCKKqn91uLU5

what i am missing?

maybe it is related to the fact i using nx?

i configure only --webpack-stats-json. it's look like the chromatic add to the script the output-dir option

no i cant, because from version 6 it is not posibile to use --storybook-build-dir and --only-changed together


Looks like when you pass the same argument multiple times to the chromatic-cli it breaks. We should simply allow this, and pick the last entry in the array.

@ndelangen ndelangen requested a review from tmeasday November 15, 2021 10:49
@ndelangen ndelangen self-assigned this Nov 15, 2021
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small tweak maybe?

@zol zol removed their request for review November 16, 2021 17:52
@zol
Copy link
Member

zol commented Nov 16, 2021

Progress here looks good. Sadly I don't have the time/expertise to do code reviews anymore so I've taken myself off the list here.

@ghengeveld ghengeveld changed the base branch from next to main November 26, 2021 09:14
@ghengeveld
Copy link
Member

@ndelangen What's the status of this? Perhaps you'll want to revisit something based on @tmeasday's comment?

@ndelangen
Copy link
Member Author

Very strange for the tests to start failing all of a sudden after merging in main? @ghengeveld got any ideas?

@ghengeveld
Copy link
Member

Sorry, my bad.

@ghengeveld
Copy link
Member

I don't think this PR is going to completely address the problem. For example, in this example, the --webpack-stats-json flag is getting passed to build-storybook twice, and getting rejected as a result. We could tweak the Storybook CLI to deal with that, but really Chromatic should prevent the same flag from getting set twice when it invokes build-storybook. I think we should resolve buildScriptName to the underlying command and strip --output-dir and --webpack-stats-json (and their alias / camelCase form) from that command before invoking it.

@ndelangen
Copy link
Member Author

I see, this PR is still an improvement I guess, a step into the right direction?

@ndelangen ndelangen requested a review from ghengeveld December 2, 2021 14:36
@ndelangen
Copy link
Member Author

@ghengeveld could you add your review on this?

@ndelangen ndelangen merged commit 6621c24 into main Dec 7, 2021
@ndelangen ndelangen deleted the tech/allow-many-args branch December 7, 2021 11:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants