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: Add pnpm support #19425

Merged
merged 4 commits into from Oct 18, 2022
Merged

CLI: Add pnpm support #19425

merged 4 commits into from Oct 18, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Oct 10, 2022

Issue: #19412

What I did

This adds support for the use of pnpm during CLI operations. I kept the same strategy as was used for npm, giving it a --use-pnpm flag to force its use. Otherwise, we'll check if it's installed and look for a pnpm-lock.yaml file to determine if we should use it.

Note: this also adds support for forcing npm (as well as pnpm) for sb automigrate, which was missing before.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

I didn't add any end-to-end style test here, but it is possible to verify it's working manually:

# Compile the branch
yarn task --task compile --start-from compile

# from somewhere else outside of the storybook repo:
pnpm create vite@latest --template=react

# cd into your new project and run
pnpm install

# Initialize storybook
node ../../../storybook/storybook/code/lib/cli/dist/generate.js init

You should see pnpm being used to install dependencies. Without the changes in this PR, the installation step would fail.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@IanVS Should we change this to --package-manager=npm|yarn1|yarn2|pnpm & deprecate the old flags now that we support a variety of options?

@ndelangen
Copy link
Member

I like that idea a lot @shilman

@IanVS IanVS changed the title CLI: Add PNPM support (--use-pnpm) CLI: Add PNPM support Oct 17, 2022
@IanVS IanVS requested a review from shilman October 18, 2022 01:01
@IanVS IanVS closed this Oct 18, 2022
@IanVS IanVS reopened this Oct 18, 2022
@IanVS
Copy link
Member Author

IanVS commented Oct 18, 2022

Closed by a misclick.

I wanted to say I got this note from @jonniebigodes:

I wanted to follow up with you on this pr, I've checked and it's good on my end. I've also scanned other repos in both organizations and so far there's a couple of changes in the tutorials, more specifically some translations that are still using the deprecated version. Once this is out I'll update the tutorials accordingly.

And, I've made the change suggested by @shilman. I think that's a much better approach.

@kasperpeulen
Copy link
Contributor

LGTM!

@kasperpeulen kasperpeulen self-requested a review October 18, 2022 13:48
@shilman shilman changed the title CLI: Add PNPM support CLI: Add pnpm support Oct 18, 2022
@NatoBoram
Copy link

Is there anything special we need to do to use it?

pnpx storybook init --use-pnpm
 ERROR  Command failed with exit code 1: /home/nato/.local/share/pnpm/store/v3/tmp/dlx-82902/node_modules/.bin/storybook init --use-pnpm

pnpm: Command failed with exit code 1: /home/nato/.local/share/pnpm/store/v3/tmp/dlx-82902/node_modules/.bin/storybook init --use-pnpm
    at makeError (/home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:22415:17)
    at handlePromise (/home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:22986:33)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Object.handler [as dlx] (/home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:193613:7)
    at async /home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:200906:21
    at async main (/home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:200877:34)
    at async runPnpm (/home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:201101:5)
    at async /home/nato/.cache/node/corepack/pnpm/7.15.0/dist/pnpm.cjs:201093:7

@IanVS
Copy link
Member Author

IanVS commented Nov 11, 2022

@NatoBoram it is only released in the storybook@next tag.

@NatoBoram
Copy link

NatoBoram commented Nov 11, 2022

🤦‍♂️

Thanks. For future reference, the steps are:

pnpm i -g storybook@next

pnpm create svelte@latest test_project
cd test_project
pnpm i
storybook init

@IanVS
Copy link
Member Author

IanVS commented Nov 12, 2022

That works, or you can use pnpx sb@next init (sb is an alias for storybook)

@NatoBoram
Copy link

I wasn't able to make it work with @next personally

@IanVS
Copy link
Member Author

IanVS commented Nov 14, 2022

@NatoBoram could you please open an issue with a set of reproduction steps and the error you are seeing?

@NatoBoram
Copy link

Looks like the situation changed, it works now.

The command I had used was pnpx storybook@next init but it used the @latest tag instead of @next. I don't know if I made a typo at that time or if another program got updated and fixed it. I even uninstalled storybook globally and pruned the store just to make sure it's not merely installing storybook that fixed it but it still works.

Regardless, thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants