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

CLI: Don't allow root directory as static dir #14068

Merged
merged 3 commits into from
Mar 18, 2021
Merged

CLI: Don't allow root directory as static dir #14068

merged 3 commits into from
Mar 18, 2021

Conversation

oscard0m
Copy link
Contributor

Issue: #13860

Context

When a user builds storybook in standalone mode, exists the possibility to provide the root directory (/) as one of the static directories to copy from (probably by mistake). This ends up trying to copy the whole root directory which is an unexpected behavior.

What I did

  • When building statics standalone throws an error if the staticDir option provided contains the root directory: /.
  • Added test coverage for invalid staticDir value
    • when it's root directory /

How to test

  • Is this testable with Jest or Chromatic screenshots? ✅

  • Does this need a new example in the kitchen sink apps? I don't think so but since I'm lacking context on what's the scope of the examples I would like the feedback from a maintainer here.

  • Does this need an update to the documentation? Checking existing standalone.md I don't think so but if you think this is an opportunity to add a disclaimer there, this PR can be used for it 😄

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

minor changes, otherwise LGTM!

lib/core-server/src/core-presets.test.ts Outdated Show resolved Hide resolved
@oscard0m oscard0m requested a review from shilman March 14, 2021 20:53
@oscard0m
Copy link
Contributor Author

Applied minor changes @shilman

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking good. I think this variable should probably be renamed from staticDir to staticDirs because it's an array, but that's probably better for another PR. Thanks for fixing this!

@shilman shilman merged commit a748246 into storybookjs:next Mar 18, 2021
@oscard0m oscard0m deleted the do-not-allow-root-directory-as-static-dir branch March 18, 2021 13:00
@oscard0m
Copy link
Contributor Author

oscard0m commented May 5, 2021

Looking good. I think this variable should probably be renamed from staticDir to staticDirs because it's an array, but that's probably better for another PR. Thanks for fixing this!

@shilman I owed you this #14819 (comment) 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants