Fix tree shaking

[cdedreuille]

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:
evanw/esbuild#2749

📦 Published PR as canary version: 1.2.1--canary.21.bf34a28.0

✨ Test out this PR locally via:

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

[socket-security]

👍 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]

Here are a few tests with canary @storybook/[email protected]

[valentinpalkovic]

Does that mean that we don't provide cjs exports anymore?

[cdedreuille]

I found a couple of places where it says that setting "main": "./dist/index.js", is enough for cjs. But I might be wrong. What would be your recommendation?

[valentinpalkovic]

I am not that experienced with this kind of setting. @ndelangen is the best person to ask.

[ndelangen]

On what basis are we making these changes? What's the rationale? Do we know what effects these changes will have and why?

I'd expect a PR description detailing what the problem was, how the code change fixes it and potentially the alternatives and potential alternatives.

This is the first time for me looking at this codebase as far as I can remember, so I can't determine if this change is ok or now.

• removing cjs exports seems/feels wrong
• adding the "pure"-comments in source seems wrong too.
  But maybe there are valid reasons for doing this. I do not have enough context.

I do not trust the screenshots of a website I do not know the details of.
I was told it's running esbuild, but it's extremely dependent on how esbuil is configured, as well as other factors in our toolchain if the code is actually treeshaken in the end or not.
I'd like to see the actual tree-shaken code when consumed in the manager itself, and inside a manager-entry.

[cdedreuille]

@ndelangen We are not using the library anywhere right now. The question is more about is the best way to export the library in package.json. I made this change in package.json because it was recommended in a couple of articles I found but I'm definitely not an expert in that field.

• I am not removing cjs export
• Adding the pure comment is a compromise we found with @JReinhold and @valentinpalkovic to make sure forward Ref is considered pure. This PR for more context: React.forwardRef is not annotated as PURE evanw/esbuild#2749

[cdedreuille]

I guess I'm not sure what's the difference between:

"main": "./dist/index.js",
"module": "./dist/index.mjs",

and

"main": "./dist/index.js",
"exports": {
  ".": {
    "require": "./dist/index.js",
    "import": "./dist/index.mjs"
  }
}

In the later, it seem we still need "main". I can't find a clear answer on the web.

[cdedreuille]

By the way @ndelangen I brought back the change.

[ndelangen]

Adding the pure comment is a compromise we found with @JReinhold and @valentinpalkovic to make sure forward Ref is considered pure. This PR for more context: evanw/esbuild#2749

Thank you for that context & link, I agree that it makes sense then.

brought back the change

Ok, I see. I'm fine with removing it, if we're sure that's the right thing to do, and we can argue why, and preferable demonstrate.

package.json exports fields are complex, because they are used by a variety of tools in a variety of ways depending on multiple variables

[Georgegriff]

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

[cdedreuille]

@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]

@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

• @storybook/components is imported by @storybook/addon-a11y and also @storybook/ui i think too (haven't traced that dependency through.

This is the error we hit:

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