-
Notifications
You must be signed in to change notification settings - Fork 7
build: fix it :D #157
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
build: fix it :D #157
Conversation
f760b0a to
fbdb8f7
Compare
WalkthroughAdds SKIP_ENV_VALIDATION=true to the build script, switches several environment-related imports and files to .mjs, conditions client/server env validation in next.config.mjs on that variable, updates tsconfig includes to .mjs files, and exports the Next.js config default. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CLI as npm/yarn
participant Next as Next Build
participant Config as next.config.mjs
participant Env as Env validators (server/client)
Dev->>CLI: run build
CLI->>Next: SKIP_ENV_VALIDATION=true next build
Next->>Config: load next.config.mjs
alt SKIP_ENV_VALIDATION !== "true"
Config->>Env: validateServerEnv()
Config->>Env: validateClientEnv()
Env-->>Config: validation results
else SKIP_ENV_VALIDATION === "true"
Note right of Config #cfe2ff: env validation skipped
end
Config-->>Next: config ready
Next-->>Dev: build output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
8-8: Consider cross-platform compatibility for environment variables.The syntax
SKIP_ENV_VALIDATION=true next buildworks on Unix-like systems (Linux, macOS) but will fail on Windows. Consider using thecross-envpackage for cross-platform compatibility.First, install
cross-envas a dev dependency:npm install --save-dev cross-envThen apply this diff to make the build script cross-platform:
- "build": "SKIP_ENV_VALIDATION=true next build", + "build": "cross-env SKIP_ENV_VALIDATION=true next build",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
next.config.mjs(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
next.config.mjs (2)
src/lib/env/server.js (1)
validateServerEnv(33-64)src/lib/env/client.js (1)
validateClientEnv(14-39)
🪛 GitHub Actions: Build and Lint
next.config.mjs
[error] 1-1: SyntaxError: The requested module './src/lib/env/client.js' does not provide an export named 'default'. Build failed while loading next.config.mjs during 'npm run build'.
🔇 Additional comments (1)
next.config.mjs (1)
7-10: LGTM! Conditional validation logic is correct.The guard correctly skips environment validation when
SKIP_ENV_VALIDATIONis set to"true", which aligns with the build script change in package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
next.config.js (1)
4-7: Consider removing redundant skip logic.The conditional block works correctly. However, based on the relevant code snippets, both
validateServerEnv()andvalidateClientEnv()already callshouldSkipValidation()internally and return early with a warning if validation should be skipped. This creates two layers of the same check.Consider one of these approaches:
Option 1 (Recommended): Remove the outer conditional and rely solely on the validation functions' internal skip logic:
-if (process.env.SKIP_ENV_VALIDATION !== "true") { - validateServerEnv(); - validateClientEnv(); -} +validateServerEnv(); +validateClientEnv();Option 2: If the outer check serves a different purpose (e.g., avoiding import side effects), keep it but document why both layers exist.
This simplification would improve maintainability by keeping the skip logic in one place and reducing potential confusion for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
next.config.js (2)
src/lib/env/server.js (1)
validateServerEnv(33-64)src/lib/env/client.js (1)
validateClientEnv(14-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
next.config.js (1)
1-2: LGTM!The import statements are correct and properly reference the validation functions with explicit
.jsextensions.
8b20d9f to
2efb232
Compare
2efb232 to
3465791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
next.config.mjs (1)
4-7: Redundant validation skip check.Both
validateServerEnvandvalidateClientEnvinternally callshouldSkipValidation()at their start (seesrc/lib/env/server.jslines 33-36 andsrc/lib/env/client.jslines 14-17), making this outer conditional check redundant. While defensive, this duplication means the skip logic exists in two places.Consider one of these approaches:
- Remove the outer conditional and rely solely on the internal checks within the validation functions.
- Keep the outer conditional and remove the internal
shouldSkipValidation()checks to centralize the logic here.Additionally, the string comparison
!== "true"is strict. Values like "TRUE", "1", or "yes" won't skip validation. If broader flexibility is desired, consider a case-insensitive check or accepting multiple truthy values.Based on relevant code snippets from
src/lib/env/server.jsandsrc/lib/env/client.js.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
next.config.mjs(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
next.config.mjs (2)
src/lib/env/server.js (1)
validateServerEnv(33-64)src/lib/env/client.js (1)
validateClientEnv(14-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
next.config.mjs (1)
1-2: Import syntax correctly fixed.The named imports now match the exports from the validation modules, addressing the previous build failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tsconfig.json (1)
29-31: Consider switching back to a glob for env files. Explicit listing is accurate but will require manual updates whenever new.mjsfiles are added; using"src/lib/env/*.mjs"ensures future files are picked up automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
next.config.mjs(1 hunks)src/lib/env/client.mjs(1 hunks)src/lib/env/server.mjs(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
next.config.mjs (2)
src/lib/env/server.mjs (1)
validateServerEnv(33-64)src/lib/env/client.mjs (1)
validateClientEnv(14-39)
🔇 Additional comments (4)
src/lib/env/client.mjs (1)
2-2: LGTM!The import path update to
.mjsextension is correct and aligns with the ES module migration across the codebase.src/lib/env/server.mjs (1)
2-2: LGTM!The import path update to
.mjsextension is consistent with the ES module migration.next.config.mjs (2)
1-7: LGTM!The import paths have been correctly updated to
.mjsextensions, and the conditional validation logic properly guards environment checks during build whenSKIP_ENV_VALIDATION=true. This addresses the previous build failure.
12-12: LGTM!The default export is now properly configured.
MantisClone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bassgeta Approved 👍 Pending comment resolution 🚧
The PR Description does not adequately explain the problem and is missing details about the solution.
Something like:
# Problem
#149 broke the build because it introduced new .js files for environment validation.
# Solution
Change the 3 environment validation files from `.js` to `.mjs` so they're processed as JS Modules (`import`/`export` syntax), not CommonJS (`requires` syntax).
# Bonus
Added option to skip environment validation for local development
Problem
#149 broke the build because it introduced new .js files for environment validation.
Solution
Change the 3 environment validation files from
.jsto.mjsso they're processed as JS Modules (import/exportsyntax), not CommonJS (requiressyntax).And it works 🎉
Summary by CodeRabbit