storybook public #200
storybook public #200
Conversation
✅ Deploy Preview for fireproof-ai-builder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main concerns: 1) build scripts (DISABLE_REACT_ROUTER=true, cp -r) are Unix-specific and break cross-platform support; 2) Storybook cache-header patterns are overly narrow and may miss real output files, reducing caching effectiveness. Everything else looks reasonable.
Additional notes (1)
- Performance |
netlify.toml:90-99
Storybook 7’s generated JS & CSS usually live directly under/storybook/(e.g.sb-preview.<hash>.js) or/storybook/assets/*, not/storybook/static/js/*. The current header rules risk missing those files, so they may get default (short) caching instead of the intended 1-year immutable cache.
Summary of changes
What changed in this PR
- netlify.toml
- Switched build command to
pnpm run build:all. - Added SPA–style redirects for
/storybookand/storybook/*so Storybook is served correctly on Netlify. - Added caching headers for Storybook‐related HTML, JS, CSS, and media assets.
- Switched build command to
- package.json
- Re-wired
buildto run a newbuild:allscript that builds the app and Storybook, then copies Storybook into the final publish directory. - Added
build:app,build:storybook, anddev:with-storybookconveniences. - Build scripts now rely on an env-var flag (
DISABLE_REACT_ROUTER) andcp -rto orchestrate the Storybook build/copy.
- Re-wired
| "build": "react-router build", | ||
| "build": "pnpm build:all", | ||
| "build:app": "react-router build", | ||
| "build:storybook": "DISABLE_REACT_ROUTER=true storybook build --output-dir storybook-static", |
There was a problem hiding this comment.
Using Unix-style env-var assignment (DISABLE_REACT_ROUTER=true …) will fail on Windows shells (cmd.exe, PowerShell). The team has been trying to keep scripts cross-platform, so this breaks that convention and may frustrate contributors on Windows.
Suggestion
Consider prepending the command with cross-env:
"build:storybook": "cross-env DISABLE_REACT_ROUTER=true storybook build --output-dir storybook-static"cross-env is already a common dev dependency in many repos and makes the script platform-agnostic. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| "build": "pnpm build:all", | ||
| "build:app": "react-router build", | ||
| "build:storybook": "DISABLE_REACT_ROUTER=true storybook build --output-dir storybook-static", | ||
| "build:all": "pnpm build:app && pnpm build:storybook && cp -r storybook-static build/client/storybook", |
There was a problem hiding this comment.
cp -r is another Unix-only command; Windows users won’t have it. This undermines the goal of pnpm build:all being the single build script that works everywhere.
Suggestion
Use a cross-platform shim such as shx (shx cp -r …) or cpx instead of plain cp:
"build:all": "pnpm build:app && pnpm build:storybook && shx cp -r storybook-static build/client/storybook"Add shx as a dev dependency if it’s not already present. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
No description provided.