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

CLI refactor #4168

Merged
merged 14 commits into from
Sep 16, 2018
Merged

CLI refactor #4168

merged 14 commits into from
Sep 16, 2018

Conversation

Keraito
Copy link
Contributor

@Keraito Keraito commented Sep 11, 2018

Refactoring the CLI as an initial step to make a richer CLI: #1108 (comment).

It might be needed to introduce some kind of way to persist data down the road for stuff like --use-npm, kinda depends on how much options are shared across the different commands.

Running storybook will display the help menu with all the commands and options.

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.

LGTM!

logger.log(chalk.yellow('To install storybook, use the following command instead:\n'));
codeLog(['storybook init']);
logger.log();
process.exit(0);
Copy link
Member

Choose a reason for hiding this comment

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

The depreciation should actually work. I assume getstorybook should do what storybook init does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds logical, will do

let projectType;
program
.command('build')
.description('Build the Storybook server')
Copy link
Member

Choose a reason for hiding this comment

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

maybe something like "Build the Storybook static app" ?

logger.log();
paddedLog('There seems to be a storybook already available in this project.');
paddedLog('Apply following command to force:\n');
codeLog(['getstorybook -f']);
Copy link
Member

Choose a reason for hiding this comment

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

getstorybook ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops slipped through, will change

default:
paddedLog(`We couldn't detect your project type. (code: ${projectType})`);
paddedLog(
"Please make sure you are running the `getstorybook` command in your project's root directory."
Copy link
Member

Choose a reason for hiding this comment

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

getstorybook ?

"Please make sure you are running the `getstorybook` command in your project's root directory."
);
paddedLog(
'You can also install storybook for plain HTML snippets with `getstorybook --html` or follow some of the slow start guides: https://storybook.js.org/basics/slow-start-guide/'
Copy link
Member

Choose a reason for hiding this comment

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

getstorybook ?

@codecov
Copy link

codecov bot commented Sep 12, 2018

Codecov Report

Merging #4168 into master will decrease coverage by 0.18%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4168      +/-   ##
==========================================
- Coverage   40.74%   40.55%   -0.19%     
==========================================
  Files         489      491       +2     
  Lines        5799     5826      +27     
  Branches      778      780       +2     
==========================================
  Hits         2363     2363              
- Misses       3062     3087      +25     
- Partials      374      376       +2
Impacted Files Coverage Δ
lib/cli/lib/yarn_spawn_sync.js 0% <0%> (ø)
lib/cli/bin/generate.js 0% <0%> (ø) ⬆️
lib/cli/lib/initiate.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0b79e7...30298fb. Read the comment docs.

@@ -18,7 +18,8 @@
"license": "MIT",
"author": "Storybook Team",
"bin": {
"getstorybook": "./bin/index.js"
"getstorybook": "./bin/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

We could also decide to keep these 2 separate, and keep all backwards compatibility in the getstorybook.js file.

That might be easier to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this locally, but when I change the entrypoint for getstorybook to something else the command is not detected, not sure what's happening here 🤔 the symlink is present tho

@ndelangen ndelangen changed the title Cli refactor CLI refactor Sep 12, 2018
@ndelangen
Copy link
Member

This is great!

@libetl
Copy link
Member

libetl commented Sep 12, 2018

That appeals to me, despite the few comments I left

Copy link
Member

@libetl libetl left a comment

Choose a reason for hiding this comment

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

tiny changes please (at least the --riot parameter)

@@ -15,7 +15,7 @@ So you can develop UI components in isolation without worrying about app specifi
```sh
npm i -g @storybook/cli
cd my-app
getstorybook
storybook init --riot
Copy link
Member

Choose a reason for hiding this comment

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

oops

Copy link
Member

Choose a reason for hiding this comment

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

--riot parameter does not exist yet

Copy link
Contributor Author

@Keraito Keraito Sep 12, 2018

Choose a reason for hiding this comment

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

I will address this in another PR, see #4168 (comment)

program
.command('start')
.description('Start the local Storybook server')
.option('-N --use-npm', 'Use NPM to start the Storybook server')
Copy link
Member

Choose a reason for hiding this comment

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

npm is widely used outside storybook in my opinion. We can identify that easily but figuring out wether we have a package-lock.json or a yarn.lock
np is able to do that (https://github.com/sindresorhus/np)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to say that at the very end of the review process, I did not see your PR earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the CLI was made, I don't think NPM had the lockfile yet (i think), that's why the flag was added to the command. On the other hand, we can also argue that what should happen when both files are present (for whatever reason). Although I definitely agree with your solution if we want to prioritize NPM over Yarn, or the other way around which happens now, it would be a great idea

Copy link
Member

Choose a reason for hiding this comment

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

also npm-shrinkwrap.json 😅

lib/cli/lib/initiate.js Show resolved Hide resolved
@libetl
Copy link
Member

libetl commented Sep 13, 2018

thanks

@Keraito
Copy link
Contributor Author

Keraito commented Sep 14, 2018

Anything else blocking this? 😊

@igor-dv igor-dv merged commit 4ee8dc4 into storybookjs:master Sep 16, 2018
This was referenced Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants