fix: resolve npm run dev errors for local development #17
Conversation
Update server to only serve static files in production mode, preventing ENOENT errors when client/build doesn't exist during development. Add TypeScript configuration updates for react-scripts compatibility. Note: Local developers need to add DANGEROUSLY_DISABLE_HOST_CHECK=true to client/.env to bypass webpack dev server host validation.
Summary of ChangesHello @virajago, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the local development experience by resolving critical Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request fixes errors for local development by conditionally serving static files only in production and updating TypeScript configurations. The changes are generally good, but I've identified a potential issue in how the environment is checked for serving static files. My suggestion aims to make this check more robust to prevent crashes in different environments.
| const isDevelopment = process.env.NODE_ENV === "development"; | ||
|
|
||
| if (!isDevelopment) { | ||
| const buildPath = path.join(__dirname, "../client/build"); | ||
|
|
||
| const setCustomCacheControl = (res: Response, filePath: string) => { | ||
| // For HTML files, always revalidate to get the latest version | ||
| if (path.extname(filePath) === ".html") { | ||
| res.setHeader("Cache-Control", "no-cache, must-revalidate"); | ||
| } else { | ||
| // For all other static assets (JS, CSS, images), cache for a long time | ||
| res.setHeader("Cache-Control", "public, max-age=31536000, immutable"); | ||
| } | ||
| }; | ||
|
|
||
| // Catch-all handler for React routing | ||
| app.get("*", (req: Request, res: Response) => { | ||
| const indexPath = path.join(buildPath, "index.html"); | ||
| res.setHeader( | ||
| "Cache-Control", | ||
| "no-store, no-cache, must-revalidate, proxy-revalidate" | ||
| app.use( | ||
| express.static(buildPath, { | ||
| setHeaders: setCustomCacheControl, | ||
| etag: true, | ||
| }) | ||
| ); | ||
| res.sendFile(indexPath); | ||
| }); | ||
|
|
||
| // Catch-all handler for React routing | ||
| app.get("*", (req: Request, res: Response) => { | ||
| const indexPath = path.join(buildPath, "index.html"); | ||
| res.setHeader( | ||
| "Cache-Control", | ||
| "no-store, no-cache, must-revalidate, proxy-revalidate" | ||
| ); | ||
| res.sendFile(indexPath); | ||
| }); | ||
| } else { | ||
| logger.info("Development mode: static files will be served by webpack-dev-server on port 3000"); | ||
| } |
There was a problem hiding this comment.
The current logic for serving static files is based on NODE_ENV not being 'development'. This can cause the server to crash with an ENOENT error if NODE_ENV is not set, or if it's set to another non-production value like 'test', because it will try to serve files from a build directory that doesn't exist.
A more robust approach is to explicitly check for 'production' to serve static files, and 'development' to log the info message. This makes the behavior predictable across all environments and prevents crashes when NODE_ENV is not set.
if (process.env.NODE_ENV === "production") {
const buildPath = path.join(__dirname, "../client/build");
const setCustomCacheControl = (res: Response, filePath: string) => {
// For HTML files, always revalidate to get the latest version
if (path.extname(filePath) === ".html") {
res.setHeader("Cache-Control", "no-cache, must-revalidate");
} else {
// For all other static assets (JS, CSS, images), cache for a long time
res.setHeader("Cache-Control", "public, max-age=31536000, immutable");
}
};
app.use(
express.static(buildPath, {
setHeaders: setCustomCacheControl,
etag: true,
})
);
// Catch-all handler for React routing
app.get("*", (req: Request, res: Response) => {
const indexPath = path.join(buildPath, "index.html");
res.setHeader(
"Cache-Control",
"no-store, no-cache, must-revalidate, proxy-revalidate"
);
res.sendFile(indexPath);
});
} else if (process.env.NODE_ENV === "development") {
logger.info("Development mode: static files will be served by webpack-dev-server on port 3000");
}
Summary
npm run devin development modeChanges
allowJsandnoFallthroughCasesInSwitchcompiler optionsTest plan
npm run devsuccessfully without ENOENT errorsNotes
Local developers need to add
DANGEROUSLY_DISABLE_HOST_CHECK=truetoclient/.envto bypass webpack dev server host validation (not included in this PR as .env files are gitignored).