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

Handle multiple -p port arguments #1627

Merged

Conversation

gpittarelli
Copy link
Contributor

@gpittarelli gpittarelli commented Aug 8, 2017

Issue: Currently, if you run start-storybook and specify the port with -p multiple times, it errors out with Error: port to run Storybook is required! This is due to a quirk of how commander passes multiple args to the option formatter parameter, leading to parseInt getting called with a weird 2nd parameter and ultimately program.port being set to NaN. See, for example, tj/commander.js#523

This bug probably came up because the example on commander's README erroneously uses parseInt in this same way.

This is annoying when I have multiple checkouts of a project using storybook with the automatically added npm script: "storybook": "start-storybook -p 6006". If I want to run multiple storybook instances, I would expect to be able to do npm run storybook in one checkout, and then npm run storybook -- -p 6007 in the other to override the port number (It would expand to start-storybook -p 6006 -p 6007)

Most CLI programs interpret extra instances of an argument to override previous instances. For example: ls -l --block-size=1 --block-size=16 uses 16 as the block size.

What I did

Changed the commander option parsing function to pass only the first parameter to parseInt and call it with a specified radix.

How to test

Get this branch's start-storybook linked into some project with storybook setup, then start-storybook -p 6006 -p 6007 should start storybook running on port 6007.

@gpittarelli gpittarelli changed the title Fix usage of parseInt for commander argument Handle multiple -p port arguments Aug 8, 2017
@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 8, 2017

I'd prefer to do something like what create-react-app does: https://github.com/facebookincubator/create-react-app/blob/master/packages/react-dev-utils/WebpackDevServerUtils.js#L380

Basically, they just prompt with suggestion for another port if the default one is unavailable

@Hypnosphi
Copy link
Member

This particular PR should be merged anyway because passing parseInt as a function argument almost never is what you actually want to do.

@Hypnosphi Hypnosphi merged commit 4d4581b into storybookjs:master Aug 8, 2017
@gpittarelli gpittarelli deleted the commander-multiple-port-number-fix branch August 8, 2017 23:50
@Hypnosphi Hypnosphi added the bug label Aug 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants