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

Run nyc with correct packager manager command #271

Conversation

ferdinandhummel-gph
Copy link
Contributor

@ferdinandhummel-gph ferdinandhummel-gph commented Feb 28, 2023

fixes #266
runs nyc command using the correct package manager exec command to fix compatibility with pnpm

📦 Published PR as canary version: 0.13.1--canary.271.8c62e28.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

}

// Copied from github.com/nrwl/nx/blob/1975181c200eb288221c8beb94e268fe9659cc26/packages/nx/src/utils/package-manager.ts#L48-106
function getPackageManagerCommand(packageManager = detectPackageManager()) {
Copy link
Member

Choose a reason for hiding this comment

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

What if people are running this in a monorepo that only has a package.json at the root level? If they're using yarn workspaces, for instance, this function will endup saying that it's an npm project, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All scripts are usually run with cwd set to same directory as the package.json (eg. npm run ... automatically takes care of that even if you run it from a directory within your porject).
We are using a nx mono repository with one single package.json in the root of the repo, tested it and works fine

@gilbarbara
Copy link

Hey @yannbf
Are there any other blockers here? I'm running into this problem with pnpm.

@yannbf
Copy link
Member

yannbf commented Oct 8, 2023

Hey @yannbf Are there any other blockers here? I'm running into this problem with pnpm.

Hey Gil! This contribution does not account for certain monorepo settings because it doesn't find the files up, e.g. executing a script in a directory that has a package.json but not a .yarnrc file, which is higher in the tree. I'll try to improve this next week!

@watcher46
Copy link

Maybe you can get some inspiration from this repo: https://github.com/egoist/detect-package-manager

@socket-security
Copy link

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@storybook/addon-coverage 0.0.7...0.0.6 None +0/-0 23.5 kB yannbf

@yannbf
Copy link
Member

yannbf commented Oct 12, 2023

Thanks for the suggestion @watcher46!

I continued the work on this PR and I think it's done. However I'm creating another PR to allow for the canary release workflow to be done, which wasn't possible in the fork from this PR. Please follow that PR instead: #364

For the ones interested, please try the canary and provide feedback on the other PR:

@storybook/[email protected]

Once I have proper feedback that this fixed your issues, I will cut a release.

Thank you!

@yannbf yannbf closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-storybook --coverage cmd fails with pnpm
4 participants