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

Benchmarks: Fix @storybook/react-native-web failing to benchmark #29830

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Dec 6, 2024

What I did

Forced installation of packages, ignoring peer dependency warnings from npm, because we're not installing peer dependencies anyway.

Reason

react-native doesn't yet support React 19

https://github.com/facebook/react-native/blob/main/packages/react-native/package.json#L102

However that is what our dependencies install because we have a peer on ^18.0.0 || ^19.0.0-beta.
And package benching uses npm, which is strict on peer deps mismatch.

image

This is usually not a problem, because projects have explicit dependencies on React 18:

"react": "18.3.1",
"react-dom": "18.3.1"

That works, because all packages have an overlap with that. Users will always have these dependencies, and so does the sandboxes. But our virtual bench packages don't depend on react, so npm chooses the highest of ^18.0.0 || ^19.0.0-beta which is 19.0.0.

So another solution would be to add overrides with React 18, but just using force seems more future proof, we'd 100 % forget to change this when upgrading the monorepo to React 19.

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-storybook/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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.7 MB 77.7 MB -4.28 kB -3.73 0%
initSize 130 MB 130 MB 14.7 kB -1.2 0%
diffSize 52.4 MB 52.5 MB 19 kB 1.94 0%
buildSize 6.75 MB 6.75 MB 0 B -0.65 0%
buildSbAddonsSize 1.51 MB 1.51 MB 0 B - 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.86 MB 1.86 MB 0 B 1.33 0%
buildSbPreviewSize 0 B 0 B 0 B -0.65 -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.57 MB 3.57 MB 0 B -0.65 0%
buildPreviewSize 3.19 MB 3.19 MB 0 B 0.65 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 6.8s 7.9s 1.1s -0.9 14.4%
generateTime 25.5s 23.2s -2s -250ms 1.56 🔰-9.7%
initTime 15.9s 17.7s 1.8s 2.31 🔺10.4%
buildTime 8.1s 9.5s 1.4s 0.41 14.8%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.4s 5.2s -136ms 0.13 -2.6%
devManagerResponsive 3.8s 3.8s -1ms 0.31 0%
devManagerHeaderVisible 555ms 630ms 75ms 0.7 11.9%
devManagerIndexVisible 629ms 664ms 35ms 0.17 5.3%
devStoryVisibleUncached 1.9s 1.8s -36ms 0.55 -1.9%
devStoryVisible 627ms 666ms 39ms 0.24 5.9%
devAutodocsVisible 408ms 573ms 165ms 1.24 🔺28.8%
devMDXVisible 562ms 525ms -37ms 0.04 -7%
buildManagerHeaderVisible 549ms 527ms -22ms -0.49 -4.2%
buildManagerIndexVisible 622ms 604ms -18ms -0.26 -3%
buildStoryVisible 498ms 489ms -9ms -0.56 -1.8%
buildAutodocsVisible 440ms 470ms 30ms 0.32 6.4%
buildMDXVisible 404ms 426ms 22ms -0.2 5.2%

Greptile Summary

Based on the provided information, I'll create a concise summary of the PR changes:

Modified package benchmarking script to handle React version compatibility issues by forcing npm installations to ignore peer dependency warnings.

  • Added --force flag to npm install command in scripts/bench/bench-packages.ts to bypass peer dependency checks
  • Addresses react-native compatibility issue with React 19 beta versions
  • Avoids need for explicit React 18 overrides in benchmark packages
  • Future-proofs solution for eventual React 19 upgrade
  • Maintains existing benchmark functionality while fixing @storybook/react-native-web failures

@JReinhold JReinhold changed the title force install dependencies to ignore peer dep warnings Benchmarks: Fix @storybook/react-native-web failing to benchmark Dec 6, 2024
Copy link

nx-cloud bot commented Dec 6, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a16f085. 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 target

Sent with 💌 from NxCloud.

@JReinhold JReinhold self-assigned this Dec 6, 2024
@JReinhold JReinhold requested a review from yannbf December 6, 2024 10:09
@JReinhold JReinhold added build Internal-facing build tooling & test updates ci:normal labels Dec 6, 2024
@JReinhold JReinhold marked this pull request as ready for review December 6, 2024 10:10
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@storybook-pr-benchmarking
Copy link

storybook-pr-benchmarking bot commented Dec 6, 2024

Package Benchmarks

Commit: a16f085, ran on 6 December 2024 at 13:00:10 UTC

The following packages have significant changes to their size or dependencies:

@storybook/builder-webpack5

Before After Difference
Dependency count 223 224 🚨 +1 🚨
Self size 79 KB 79 KB 0 B
Dependency size 29.65 MB 29.67 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

@storybook/core

Before After Difference
Dependency count 46 47 🚨 +1 🚨
Self size 19.05 MB 19.05 MB 0 B
Dependency size 14.30 MB 14.32 MB 🚨 +16 KB 🚨
Bundle Size Analyzer Link Link

@storybook/ember

Before After Difference
Dependency count 250 251 🚨 +1 🚨
Self size 22 KB 22 KB 0 B
Dependency size 32.05 MB 32.08 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

@storybook/html-webpack5

Before After Difference
Dependency count 233 234 🚨 +1 🚨
Self size 6 KB 6 KB 0 B
Dependency size 30.20 MB 30.22 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs

Before After Difference
Dependency count 585 586 🚨 +1 🚨
Self size 464 KB 464 KB 0 B
Dependency size 83.86 MB 83.88 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/preact-webpack5

Before After Difference
Dependency count 231 232 🚨 +1 🚨
Self size 6 KB 6 KB 0 B
Dependency size 29.77 MB 29.80 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

@storybook/react-webpack5

Before After Difference
Dependency count 309 310 🚨 +1 🚨
Self size 6 KB 6 KB 0 B
Dependency size 40.95 MB 40.96 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/server-webpack5

Before After Difference
Dependency count 241 242 🚨 +1 🚨
Self size 14 KB 14 KB 0 B
Dependency size 31.18 MB 31.20 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

@storybook/svelte-webpack5

Before After Difference
Dependency count 296 297 🚨 +1 🚨
Self size 6 KB 6 KB 0 B
Dependency size 37.77 MB 37.79 MB 🚨 +23 KB 🚨
Bundle Size Analyzer Link Link

@storybook/vue3-vite

Before After Difference
Dependency count 107 108 🚨 +1 🚨
Self size 16 KB 16 KB 0 B
Dependency size 42.54 MB 42.55 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/vue3-webpack5

Before After Difference
Dependency count 482 483 🚨 +1 🚨
Self size 6 KB 6 KB 0 B
Dependency size 54.18 MB 54.19 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/web-components-webpack5

Before After Difference
Dependency count 231 232 🚨 +1 🚨
Self size 5 KB 5 KB 0 B
Dependency size 29.82 MB 29.85 MB 🚨 +22 KB 🚨
Bundle Size Analyzer Link Link

storybook

Before After Difference
Dependency count 47 48 🚨 +1 🚨
Self size 22 KB 22 KB 0 B
Dependency size 33.35 MB 33.37 MB 🚨 +16 KB 🚨
Bundle Size Analyzer Link Link

sb

Before After Difference
Dependency count 48 49 🚨 +1 🚨
Self size 1 KB 1 KB 0 B
Dependency size 33.37 MB 33.39 MB 🚨 +16 KB 🚨
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 388 389 🚨 +1 🚨
Self size 484 KB 484 KB 🎉 -102 B 🎉
Dependency size 74.92 MB 74.94 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 268 269 🚨 +1 🚨
Self size 612 KB 612 KB 0 B
Dependency size 64.91 MB 64.93 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 105 106 🚨 +1 🚨
Self size 1.11 MB 1.11 MB 0 B
Dependency size 42.49 MB 42.50 MB 🚨 +16 KB 🚨
Bundle Size Analyzer Link Link

@storybook/preset-vue3-webpack

Before After Difference
Dependency count 365 366 🚨 +1 🚨
Self size 9 KB 9 KB 0 B
Dependency size 45.10 MB 45.12 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Dec 6, 2024
@yannbf yannbf merged commit 10c4bfa into next Dec 6, 2024
57 of 58 checks passed
@yannbf yannbf deleted the jeppe/fix-bench-rnw branch December 6, 2024 13:14
@yannbf yannbf removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates ci:normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants