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

Remove mdx1-csf as addon-docs peer dependency #21935

Closed
dartess opened this issue Apr 5, 2023 · 4 comments · Fixed by #22038
Closed

Remove mdx1-csf as addon-docs peer dependency #21935

dartess opened this issue Apr 5, 2023 · 4 comments · Fixed by #22038
Assignees

Comments

@dartess
Copy link
Contributor

dartess commented Apr 5, 2023

Is your feature request related to a problem? Please describe

trim@0.0.1 installing by default and leads to audit problems

I am using @storybook/addon-docs@7.0.2

After installation, I get a warning from npm and an error in the audit, since trim@0.0.1 is installed with it

└─┬ @storybook/addon-docs@7.0.2
  └─┬ @storybook/mdx1-csf@1.0.0
    └─┬ @mdx-js/mdx@1.6.22
      └─┬ remark-parse@8.0.3
        └── trim@0.0.1

As I can see the @storybook/mdx1-csf dependency is in peerDependencies. But they are installed by default via NPM, as I understand it. Even if the "optional" flag is present.

I also see that this dependency is being used by the legacyMdx1 condition.

Describe the solution you'd like

It seems possible before using a dependency to check for its existence. If it is not there, ask the user to install it themselves. You can also refine the migration process to the new version, as I remember, CLI asks if I will use the legacy version of MDX.

Describe alternatives you've considered

No response

Are you able to assist to bring the feature to reality?

yes, I can

Additional context

No response

@shilman
Copy link
Member

shilman commented Apr 5, 2023

@dartess thanks for raising this. that really sucks!

I didn't realize that npm even installs optional dependencies. Making it a non-dependency will break strict package managers like pnpm/yarn pnp, but I agree it's better to do that than to give a security warning by default in npm.

@shilman shilman self-assigned this Apr 5, 2023
@shilman shilman changed the title [Feature Request]: don't install trim@0.0.1 Remove mdx1-csf as addon-docs peer dependency Apr 5, 2023
@dartess
Copy link
Contributor Author

dartess commented Apr 12, 2023

@shilman I'm sorry I misled you.
I removed node_modules and package-lock.json and then reinstalled it. Now trim is not installed.

@dartess dartess closed this as completed Apr 12, 2023
@shilman
Copy link
Member

shilman commented Apr 12, 2023

Ta-da!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.2 containing PR #22038 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb@next upgrade --tag future

@shilman
Copy link
Member

shilman commented Apr 12, 2023

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.4 containing PR #22038 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb@latest upgrade

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

Successfully merging a pull request may close this issue.

2 participants