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

Revert "Revert "Build: Migrate repo to Nx 20"" #29821

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Dec 5, 2024

Attempt at understanding about the failure this PR caused, so we can bring it back once again!

Reverts #29820

Greptile Summary

This PR reverts a previous reversion, re-attempting to migrate the Storybook repository to Nx 20 with fixes for CI failures, including significant updates to build configuration and dependency management.

  • Upgraded Nx packages from 18.0.6 to 20.1.4 in code/package.json and modified project configurations
  • Removed code/migrations.json to clear migration history for a fresh start
  • Added .nx/workspace-data to gitignore files and updated Vite config timestamp patterns
  • Fixed invalid project configurations in code/frameworks/angular/project.json and code/renderers/vue3/project.json
  • Modified build tasks in scripts/tasks/ to use npx nx instead of direct nx calls

@yannbf yannbf added build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job. labels Dec 5, 2024
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.

12 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -188,9 +188,10 @@
"esbuild": "^0.18.0 || ^0.19.0 || ^0.20.0 || ^0.21.0 || ^0.22.0 || ^0.23.0 || ^0.24.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Wide version range for esbuild could lead to inconsistent builds. Consider pinning to specific version

Comment on lines 181 to +182
"type-fest": "~2.19",
"typescript": "^5.4.5",
"typescript": "5.4.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Pinning TypeScript to exact version 5.4.5 while other dependencies use ranges could cause version conflicts with other packages that depend on TypeScript

Comment on lines +10 to +11
const linkCommand = `npx nx affected -t check ${parallel}`;
const nolinkCommand = `npx nx affected -t check -c production ${parallel}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: using npx requires package resolution on each run, which may impact performance in development

@@ -12,8 +12,8 @@ const amountOfVCPUs = 4;
const parallel = `--parallel=${process.env.CI ? amountOfVCPUs - 1 : maxConcurrentTasks}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: using amountOfVCPUs - 1 on CI but full maxConcurrentTasks locally could cause inconsistent behavior between environments

Copy link

nx-cloud bot commented Dec 6, 2024

☁️ Nx Cloud Report

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

@storybook-pr-benchmarking
Copy link

Package Benchmarks

Commit: b04c5a1, ran on 6 December 2024 at 14:32:35 UTC

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

@storybook/addon-interactions

Before After Difference
Dependency count 56 56 0
Self size 129 KB 129 KB 0 B
Dependency size 12.44 MB 12.73 MB 🚨 +286 KB 🚨
Bundle Size Analyzer Link Link

@storybook/experimental-addon-test

Before After Difference
Dependency count 61 61 0
Self size 834 KB 834 KB 🚨 +5 B 🚨
Dependency size 13.86 MB 14.15 MB 🚨 +286 KB 🚨
Bundle Size Analyzer Link Link

@storybook/experimental-nextjs-vite

Before After Difference
Dependency count 87 87 0
Self size 231 KB 231 KB 0 B
Dependency size 31.28 MB 31.57 MB 🚨 +286 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs

Before After Difference
Dependency count 586 586 0
Self size 464 KB 466 KB 🚨 +2 KB 🚨
Dependency size 83.88 MB 84.16 MB 🚨 +286 KB 🚨
Bundle Size Analyzer Link Link

@storybook/test

Before After Difference
Dependency count 53 53 0
Self size 1.52 MB 1.81 MB 🚨 +286 KB 🚨
Dependency size 8.09 MB 8.09 MB 0 B
Bundle Size Analyzer Link Link

Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

LGTM

@kasperpeulen kasperpeulen merged commit 9313346 into next Dec 6, 2024
107 checks passed
@kasperpeulen kasperpeulen deleted the revert-29820-revert-29807-nx-20 branch December 6, 2024 15:45
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:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants