storybookjs / icons

Library of icons used in apps and marketing sites
MIT License
9 stars 3 forks source link

Fix tree shaking #21

Closed cdedreuille closed 10 months ago

cdedreuille commented 10 months ago

In the previous PR we introduced React.forwardRef and reformatted how svgs are structured. Because ESBuild and TSUP doesn't consider React.forwardRef as pure we had to add it locally on each components to make sure that tree shaking is working again.

Here's a reference to a similar issue and why ESBuild is not going to resolve that: https://github.com/evanw/esbuild/issues/2749

📦 Published PR as canary version: 1.2.1--canary.21.bf34a28.0
:sparkles: Test out this PR locally via: ```bash npm install @storybook/icons@1.2.1--canary.21.bf34a28.0 # or yarn add @storybook/icons@1.2.1--canary.21.bf34a28.0 ```
socket-security[bot] commented 10 months ago

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

cdedreuille commented 10 months ago

Here are a few tests with canary @storybook/icons@1.2.1--canary.21.b9bd056.0

CleanShot 2023-10-17 at 13 37 52@2x CleanShot 2023-10-17 at 13 38 21@2x CleanShot 2023-10-17 at 13 38 35@2x

Georgegriff commented 9 months ago

Heads up

I'm not sure if you do sem-ver, but as an FYI, this seemed to be a downstream breaking change for the v7 storybook stream. Because the exports in the src/index.s we're renamed, this should have been a major version of the npm package if you do sem-ver.

Our stories started breaking at run time after the minor version was pulled in from somewhere in the dependency tree. We have our storybook locked at 7.3.2 which i think is how we noticed this. Someone updated some packages in our monorepo and the lock updated from 1.2.0 to 1.2.1 and things broken

image
cdedreuille commented 8 months ago

@Georgegriff This new library was introduced in 8.0 only. We deprecated the old icon package in @storybook/components but it is still accessible until 9.0. I'm not sure how this package affect your set up in 7.x

Did you use this library already in your project?

Georgegriff commented 8 months ago

@Georgegriff This new library was introduced in 8.0 only. We deprecated the old icon package in @storybook/components but it is still accessible until 9.0. I'm not sure how this package affect your set up in 7.x

Did you use this library already in your project?

Here is an explanation of the chain of dependencies:

-@storybook/components v7.x tags has "@storybook/icons": "^1.1.6",` because this pr was released in 1.2.3, it picked up the re-wrote imports and anything relying on this package will have to fix their code, if 1.2.3 happens to be pulled in via package locks

This is the error we hit: image This used the icons, when the package exports changed in a minor version components broke, which broke the a11y plugin which broke our storybook.

Hopefully this explains how this was a breaking change, it was other @storybook packages that were broken, as a work around we've had to pin @storybook/icons to the version before this in our pnpm config to force the transitive dependency version