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

[react][storybook] Nx add NOT needed deps on new lib setup #16395

Closed
ild0tt0re opened this issue Apr 19, 2023 · 6 comments
Closed

[react][storybook] Nx add NOT needed deps on new lib setup #16395

ild0tt0re opened this issue Apr 19, 2023 · 6 comments
Assignees
Labels
outdated scope: react Issues related to React support for Nx type: bug

Comments

@ild0tt0re
Copy link

ild0tt0re commented Apr 19, 2023

Current Behavior

When we create a new react library with a storybook configuration, the following not needed libs are installed:

  • "@babel/preset-react": "^7.14.5",
  • "@rollup/plugin-url": "^7.0.0",
  • "@storybook/core-server": "^7.0.0-rc.0",
  • "@storybook/react-webpack5": "^7.0.0-rc.0",
  • "@svgr/rollup": "^6.1.2",
  • "storybook-addon-swc": "^1.1.7"
  • "swc-loader": "0.1.15",

NOTE:

  1. We are using auto-install-peers=true installation with pnpm package manager
  2. We are on storybook "7.0.2" version but it install "^7.0.0-rc.0"

Expected Behavior

Generating a new lib should not force installation of hardcoded/wrong packages different from what is installed in the monorepo, or should not install the deps at all since the package manager can be configured in different way.

GitHub Repo

No response

Steps to Reproduce

  1. generate a new react lib
  2. configure storybook

Nx Report

Node : 18.15.0
   OS   : darwin arm64
   pnpm : 8.1.1

   nx                      : 15.9.2
   @nrwl/js                : 15.9.2
   @nrwl/jest              : 15.9.2
   @nrwl/linter            : 15.9.2
   @nrwl/workspace         : 15.9.2
   @nrwl/cli               : 15.9.2
   @nrwl/cypress           : 15.9.2
   @nrwl/devkit            : 15.9.2
   @nrwl/eslint-plugin-nx  : 15.9.2
   @nrwl/next              : 15.9.2
   @nrwl/nx-plugin         : 15.9.2
   @nrwl/react             : 15.9.2
   @nrwl/rollup            : 15.9.2
   @nrwl/storybook         : 15.9.2
   @nrwl/tao               : 15.9.2
   @nrwl/web               : 15.9.2
   @nrwl/webpack           : 15.9.2
   typescript              : 4.9.5
   ---------------------------------------
   Community plugins:
   @jscutlery/semver : 2.29.3
   ngx-deploy-npm    : 5.2.0
   ---------------------------------------
   Local workspace plugins:
   	 @cloudacademy/nx-plugin

Additional Information

I understand that Nx want to check that needed packages are installed during the generation of specific lib with its tech stack, but since is hard to understand each use case and package manager configuration I would like to have an option/flag to skip the packages installation part.

Requested also here #5018

@ild0tt0re ild0tt0re changed the title [react] generate a new react new lib with storybook config, install not needed deps [react] generate a new react lib with storybook config, install not needed deps Apr 19, 2023
@ild0tt0re ild0tt0re changed the title [react] generate a new react lib with storybook config, install not needed deps [react][storybook] Nx add NOT needed deps on new lib setup Apr 19, 2023
@mandarini
Copy link
Member

Hi there @ild0tt0re ! Thanks for filing an issue! Please help me understand, if you are using Storybook, how come

"@storybook/core-server": "^7.0.0-rc.0",
"@storybook/react-webpack5": "^7.0.0-rc.0",

are not needed?

Or do you mean you need these, but with the version you already have installed?

@ild0tt0re
Copy link
Author

ild0tt0re commented Apr 19, 2023

"@storybook/core-server": "^7.0.0-rc.0",
"@storybook/react-webpack5": "^7.0.0-rc.0",

are not needed?

Or do you mean you need these, but with the version you already have installed?

Yeah, if they need to be installed I expect should be at the same version of others storybook packages, like:

"@storybook/nextjs": "7.0.2",
"@storybook/react": "7.0.2",
...

Probably only the two dependencies that you mention need to be installed, also if we have auto-install-peers=true flag setted in .npmrc file... btw they are requested by other storybook modules and all work fine also without them

@mandarini mandarini self-assigned this Apr 19, 2023
@mandarini mandarini added scope: storybook Issues related to Storybook support in Nx and removed scope: storybook Issues related to Storybook support in Nx labels Apr 19, 2023
@mandarini mandarini removed their assignment Apr 19, 2023
@mandarini
Copy link
Member

mandarini commented Apr 19, 2023

Ok, @ild0tt0re this is not only an issue with the @nrwl/storybook package. I'll fix the part where it uses the version of your existing configuration, if it's greater than version 7.0.0. The rest of the issue, I think we need to discuss!

@AgentEnder AgentEnder added the scope: storybook Issues related to Storybook support in Nx label Apr 19, 2023
@mandarini mandarini added scope: react Issues related to React support for Nx and removed scope: storybook Issues related to Storybook support in Nx labels Apr 20, 2023
@mandarini
Copy link
Member

mandarini commented Apr 20, 2023

I am changing the scope to React, here, since the extra dependencies are installed during the react:lib generation. We should see at how we should handle this, or if it's expected behaviour.

The Storybook part of this issue was fixed in above PR, which was to use the version that the user has already installed.

So, to sum up, the expected behaviour as noted by @ild0tt0re is:

(1) Generating a new lib should not force installation of hardcoded/wrong packages different from what is installed in the monorepo, or (2) should not install the deps at all since the package manager can be configured in different way.

Part 1 ("generating a new lib should not force installation of hardcoded/wrong packages different from what is installed in the monorepo") was covered by #16405 and #16408.

Part 2 is "should not install the deps at all since the package manager can be configured in different way", which is something we need to discuss if we want?

@mandarini
Copy link
Member

mandarini commented Apr 20, 2023

Actually, @ild0tt0re, I think skipPackageJson is the flag you're looking for. I am going to close this issue, but let me know if you think that above PRs and skipPackageJson do not fix your issue, and I will re-open!

@mandarini mandarini self-assigned this Apr 20, 2023
@github-actions
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: react Issues related to React support for Nx type: bug
Projects
None yet
Development

No branches or pull requests

3 participants