Skip to content

Core: Fix building Storybook deleting project root files#29371

Merged
JReinhold merged 1 commit intonextfrom
fix-29358
Oct 16, 2024
Merged

Core: Fix building Storybook deleting project root files#29371
JReinhold merged 1 commit intonextfrom
fix-29358

Conversation

@JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 16, 2024

Closes #29358

What I did

Changed the logic for cleaning the outDir (ie. storybook-static), so it now simply removes the directory and adds an empty one.

A regression was introduced in #29126 that did this when attempting to clear the outDir:

const outputDirFiles = await readdir(options.outputDir);
for (const file of outputDirFiles) {
  await rm(file, { recursive: true, force: true });
}

However the problem with that is that readdir doesn't return the full file paths, only the file names. So it would find storybook-static/index.html, return index.html, and we would then just delete ./index.html. CC @ziebam for knowledge sharing.

I've searched through the PR for other similar uses of readdir and didn't find anything.

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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.7 MB 78.7 MB 2.56 kB 3.23 0%
initSize 147 MB 148 MB 1.03 MB 0.33 0.7%
diffSize 68.3 MB 69.3 MB 1.02 MB 0.32 1.5%
buildSize 6.79 MB 6.79 MB -4.68 kB -0.13 -0.1%
buildSbAddonsSize 1.5 MB 1.5 MB 311 B 1.72 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.83 MB 1.83 MB -245 B -0.73 0%
buildSbPreviewSize 270 kB 270 kB 12 B 0.5 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.8 MB 3.8 MB 78 B 1.08 0%
buildPreviewSize 2.99 MB 2.99 MB -4.76 kB -0.15 -0.2%
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 7.3s 17.8s 10.4s 1.18 58.8%
generateTime 18.8s 20.1s 1.3s -0.29 6.9%
initTime 11.2s 12.7s 1.5s -0.77 12%
buildTime 8s 8.3s 253ms -0.47 3%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.4s 7s 562ms 0.4 8%
devManagerResponsive 4.1s 4.4s 368ms 0.19 8.2%
devManagerHeaderVisible 525ms 604ms 79ms -0.19 13.1%
devManagerIndexVisible 560ms 631ms 71ms -0.25 11.3%
devStoryVisibleUncached 892ms 1s 141ms 0.03 13.6%
devStoryVisible 561ms 629ms 68ms -0.28 10.8%
devAutodocsVisible 544ms 491ms -53ms -0.6 -10.8%
devMDXVisible 504ms 465ms -39ms -1.02 -8.4%
buildManagerHeaderVisible 530ms 477ms -53ms -0.87 -11.1%
buildManagerIndexVisible 559ms 546ms -13ms -0.36 -2.4%
buildStoryVisible 560ms 546ms -14ms -0.81 -2.6%
buildAutodocsVisible 496ms 475ms -21ms -0.33 -4.4%
buildMDXVisible 502ms 440ms -62ms -0.69 -14.1%

Greptile Summary

This PR simplifies the output directory cleaning process in the Storybook build, addressing an issue where files were being incorrectly deleted during subsequent builds.

  • Modified code/core/src/core-server/build-static.ts to remove and recreate the entire output directory
  • Fixed a bug where index.html and other files were accidentally deleted when building Storybook multiple times
  • Replaced the previous logic that used readdir and rm with a more straightforward approach
  • Ensures consistent and reliable cleaning of the output directory before each build
  • Resolves issue [Bug]: [8.4.0-alpha.7] index.html is deleted when storybook is built twice #29358 where index.html was deleted when Storybook was built twice

@nx-cloud
Copy link

nx-cloud bot commented Oct 16, 2024

☁️ Nx Cloud Report

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

Sent with 💌 from NxCloud.

@JReinhold JReinhold self-assigned this Oct 16, 2024
@JReinhold JReinhold marked this pull request as ready for review October 16, 2024 10:45
@JReinhold JReinhold requested a review from ndelangen October 16, 2024 10:45
@JReinhold JReinhold changed the title Core: Fix building Storybook deleting too many files Core: Fix building Storybook deleting root files Oct 16, 2024
@JReinhold JReinhold changed the title Core: Fix building Storybook deleting root files Core: Fix building Storybook deleting project root files Oct 16, 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.

LGTM

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

@pralkarz
Copy link

Good catch, sorry for the regression and thanks for the ping! 💪

@JReinhold JReinhold merged commit 921c971 into next Oct 16, 2024
@JReinhold JReinhold deleted the fix-29358 branch October 16, 2024 12:28
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: [8.4.0-alpha.7] index.html is deleted when storybook is built twice

3 participants