Add @storybook/icons as a regular dependency

[valentinpalkovic]

Closes N/A

What I did

I moved @storybook/icon from devDependencies into the dependencies section since it isn't a mono repo dependency and doesn't get prebundled. It caused issues with yarn pnp

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

[github-actions]

	Fails
🚫	PR title must be in the format of "Area: Summary", With both Area and Summary starting with a capital letter Good examples: - "Docs: Describe Canvas Doc Block" - "Svelte: Support Svelte v4" Bad examples: - "add new api docs" - "fix: Svelte 4 support" - "Vue: improve docs"

Generated by 🚫 dangerJS against bd82192

[JReinhold]

This doesn't look right, it's globalized in the manager so it shouldn't be necessary.

https://github.com/storybookjs/storybook/blob/next/code/ui/manager/src/globals/globals.ts#L11

Could you elaborate on the issues you get in Yarn PnP?

[valentinpalkovic]

It isn't globalized, otherwise yarn wouldn't complain about trying to import it. I also don't think its right to globalize this package because it is a) not a mono repo package and b) the user might install a different version than the one is used in the manager. That's totally valid and would just add unnecessary confusion if we globalize it.

[JReinhold]

It isn't globalized, otherwise yarn wouldn't complain about trying to import it.

It should be, so I don't know why Yarn is complaining. What are the reproduction steps?

I also don't think its right to globalize this package because it is a) not a mono repo package

Why does this matter?

and b) the user might install a different version than the one is used in the manager. That's totally valid and would just add unnecessary confusion if we globalize it.

That is correct, this can cause confusion. However it's a perf trade off, if 10 addons install 10 different versions of the icons package, the user will have 10x all icons in their bundle.

cc @ndelangen @shilman I recall you two originally decided on this.

[valentinpalkovic]

The context: #25688 (comment)

We have no versioning rule for non-mono repo packages. Every add-on and every user can install whatever version they need. Doing some swapping in the background to use exactly one version is very magical and unintuitive.

The icons were made in a way that they are treeshakable. Therefore, this exact version matching magic is only an advantage, if different add-ons are using the same icon. Otherwise, it doesn't matter. And we are talking here about a couple of kilobytes per icon. So the bundle size reduction only matters for the same icons, not for differently used ones. I think we are sacrificing intuitiveness for marginal build size optimization of @storybook/icons.

I personally vote to remove the globalization of non mono repo packages like @storybook/icons.

[ndelangen]

Doing some swapping in the background to use exactly one version is very magical and unintuitive

If we accept it for packages like manager-api, why not accept this for icons?

The icons were made in a way that they are treeshakable.

We discussed this at length as far as i can remember, and we decided to go with the globalization approach, because it resulted in smaller bundles & fewer files to load into the browser overall.

Globalization also causes the bundler to exit out of the deep dependency graph and build faster, (though esbuild is already super fast)

We made all icons lazy-loaded, right?

Maybe I'm misremembering though, because this changed multiple times.

I remember looking at the comparison in benchmarks and this approach coming out on top.

I personally vote to remove the globalization of non mono repo packages

Yes, I get why... addon creators might depend on super later version of icons, and that version has an new icon introduced that the prebundled set doesn't have...

[valentinpalkovic]

If we accept it for packages like manager-api, why not accept this for icons?

We don't have any choice, do we? Suppose we don't globalize mono-repo packages like @storybook/manager-api. In that case, the addon can be fundamentally broken when used in Storybook because internal APIs of the mono repo packages might have changed, and the addon's @storybook/manager-api version is just not compatible with the user's Storybook version. That's not really the case for @storybook/icons, is it?

[JReinhold]

We don't have any choice, do we? Suppose we don't globalize mono-repo packages like @storybook/manager-api. In that case, the addon can be fundamentally broken when used in Storybook because internal APIs of the mono repo packages might have changed, and the addon's @storybook/manager-api version is just not compatible with the user's Storybook version. That's not really the case for @storybook/icons, is it?

This argument makes sense to me, it's not the same case for @storybook/icons.

addon creators might depend on super later version of icons, and that version has an new icon introduced that the prebundled set doesn't have...

Yeah I guess this would be the main concern. Maybe prioritising correctness (use the version that the addon author specifies) over perf (globalize) would be best here.

We made all icons lazy-loaded, right?

We did not @ndelangen, IIRC we found a bug in ESBuild that when adding the lazy loading logic it would output a non-treeshakable file, but it might be because of something else.

If we do decide to go ahead with this PR, it needs to un-globalize the package as well.

[valentinpalkovic]

The bug wasn't reproducible anymore. For now, we stay with the globalization of @storybook/icons, but we want to keep an eye on it whether it will cause issues in the future.