-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Refactor CLI #1840
Refactor CLI #1840
Conversation
Codecov Report
@@ Coverage Diff @@
## release/3.3 #1840 +/- ##
===============================================
- Coverage 22.32% 22.27% -0.06%
===============================================
Files 324 324
Lines 6319 6335 +16
Branches 803 809 +6
===============================================
Hits 1411 1411
- Misses 4297 4305 +8
- Partials 611 619 +8
Continue to review full report at Codecov.
|
I guess what we are giving up here is that previously the user's Also when we release breaking versions users will need to update the CLI. This is probably not the worst thing. I think this change is OK, but what do you think about the idea of fetching the latest version of a internal dep and comparing it to the That would cover both use cases, no? |
Sounds good, will try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
lib/cli/lib/helpers.js
Outdated
|
||
const logger = console; | ||
|
||
export async function getVersion(packageName) { | ||
const current = packageName === '@storybook/cli' ? version : devDependencies[packageName]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check that packageName.match('@storybook')
or something? Otherwise we may apply this logic to other things this package depends on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. I think it should be /storybook/.test(packageName)
though, as there are plans to use storybook
package name (@ndelangen has it)
Haven't had a chance to run the code yet but it LGTM |
This solves a couple of issues:
latest-version
is used to choose which versions of packages to install. This means that new packages and features can be used in CLI templates only after becoming stable. This PR changes it so that@storybook/cli@3.3.0-alpha.1
adds"@storybook/*": "^3.3.0-alpha.1"
, which is future-compatible meaning that versions like3.11.10
will still match this mask. This, for example, makes it possible to run smoke-tests (introduced in 3.3) when testing CLIcatch
line. See Add angular support: Storybook for Angular #1474 (comment). Here I switch it to a single handler on top levellib/cli/test/run/*
to yarn workspaces, so that after runninggetstorybook
in all the fixtures directories, a singleyarn
command can install all the needed dependencies and link local@storybook/*
packages. To do this, I also added a--skip-install
option to CLI