feat: add sideeffects to addons; #29841
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -43,6 +43,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
style: Consider setting sideEffects to ['./dist/manager.js'] since manager entries often have initialization side effects
| @@ -75,6 +75,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
style: Consider setting sideEffects to an array of framework-specific files that do have side effects instead of false
| @@ -75,6 +75,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
logic: Consider checking if any of the bundled addons (actions, backgrounds, controls etc.) have initialization side effects that need to be preserved. May need to be sideEffects: ["./dist/*/preview.js"] instead of false.
| @@ -32,6 +32,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
logic: react-confetti, framer-motion, and react-joyride may have side effects. Consider checking if these dependencies require side effects to function properly before marking the entire package as side-effect free.
| @@ -65,6 +65,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
logic: Setting sideEffects to false here may be problematic since this addon has setup files, global setup, and postinstall scripts that likely have side effects. Consider reviewing the actual side effects in the codebase before marking as side-effect free.
There was a problem hiding this comment.
19 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -42,6 +42,7 @@ | |||
| }, | |||
| "main": "dist/index.js", | |||
| "module": "dist/index.mjs", | |||
| "sideEffects": false, | |||
There was a problem hiding this comment.
logic: axe-core and accessibility testing setup often requires initialization side effects - verify that tree-shaking doesn't break the accessibility testing functionality
jonniebigodes
added
ci:daily
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 70f10ba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 105 | 102 | 🎉 -3 🎉 |
| Self size | 42 KB | 41 KB | 🎉 -435 B 🎉 |
| Dependency size | 18.01 MB | 17.86 MB | 🎉 -154 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
What I did
Add side effects to package.json, help bundlers cut unused code with treeshake. Good for esm, modules, named imports.
rollup https://rollupjs.org/configuration-options/#treeshake-modulesideeffects
webpack https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam 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>Greptile Summary
Added
sideEffects: falseto multiple Storybook addon package.json files to enable better tree-shaking optimization, with potential risks identified in certain addons.💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!