Update Node.js version support and postinstall script#233
Conversation
- Updated the GitHub Actions workflow to include "latest" in the Node.js version matrix for testing, enhancing compatibility with newer features. - Modified the postinstall script in package.json to set NODE_OPTIONS for fumadocs-mdx, ensuring proper execution without web storage. These changes aim to improve the testing environment and streamline the installation process.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds CI and docs support for Node.js v25 (latest) and updates the www app to use wrapper scripts that detect Node major version and conditionally add Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as npm / pnpm
participant Wrapper as build.js / postinstall.js
participant Runtime as Node.js
participant Tool as next / fumadocs-mdx
Runner->>Wrapper: invoke script + args
Wrapper->>Runtime: read process.version
alt major >= 25
Note right of Wrapper #DFF2E1: modify NODE_OPTIONS\n(add --no-webstorage if missing)
Wrapper->>Wrapper: inject flag into env
else major < 25
Note right of Wrapper #FFF3D9: leave NODE_OPTIONS unchanged
end
Wrapper->>Tool: spawn tool (forward stdio)
Tool-->>Wrapper: output + exit code
Wrapper-->>Runner: exit with tool's exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Revised the Node.js version support statement to include both v22 (LTS) and v25 (Current), ensuring clarity on compatibility with the latest Node.js releases. This change enhances the documentation by providing accurate information regarding supported Node.js versions.
github-actions
bot
added
docs
- Changed the postinstall script from setting NODE_OPTIONS to executing a shell script for fumadocs-mdx, improving compatibility and execution reliability during installation. This update aims to streamline the installation process and ensure proper execution of the fumadocs-mdx tool.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml(2 hunks)apps/www/package.json(1 hunks)packages/arkenv/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/README.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Document environment requirements in README files
Files:
packages/arkenv/README.md
🧠 Learnings (1)
📚 Learning: 2025-09-09T17:37:19.650Z
Learnt from: yamcodes
PR: yamcodes/arkenv#132
File: packages/arkenv/README.md:13-14
Timestamp: 2025-09-09T17:37:19.650Z
Learning: For yamcodes/arkenv project: Runtime support documentation should link to specific examples: Node.js (examples/basic), Bun (examples/with-bun), Vite (examples/with-vite-react-ts).
Applied to files:
packages/arkenv/README.md
🔇 Additional comments (3)
.github/workflows/tests.yml (2)
23-23: Appropriate matrix expansion for Node.js v25 testing.Adding
latestto the test matrix enables detection of incompatibilities with the current stable Node.js version. The test job will now run across Node.js 22 (LTS), lts/*, and latest. Combined with the postinstall workaround inapps/www/package.json, this should surface any integration issues early.
76-76: Consistent matrix expansion across build job.The test-build job mirrors the test job's node-version matrix, ensuring builds are validated across the same Node.js versions. This is appropriate for a monorepo with multiple packages.
packages/arkenv/README.md (1)
113-113: Documentation accurately reflects updated Node.js version support.The Requirements section now documents Node.js v22 (LTS) and v25 (Current), aligned with the expanded test matrix in the CI workflow. The hyperlinked examples for each runtime (Node.js, Bun, Vite) follow established documentation best practices and enhance discoverability for developers.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/www/package.json (1)
9-9: Duplicate/related concern: postinstall references bash script without cross-platform support.The postinstall script now delegates to
./bin/fumadocs-mdx.sh, which inherits the Windows compatibility issue flagged above. If you implement the Node.js wrapper suggested in the preceding comment, update this line to"postinstall": "node ./bin/fumadocs-mdx.js"for guaranteed cross-platform execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/bin/fumadocs-mdx.sh(1 hunks)apps/www/package.json(1 hunks)
⏰ 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: test-e2e
- Updated the postinstall script in package.json to execute a Node.js script instead of a shell script, enhancing compatibility. - Introduced fumadocs-mdx.js to manage Node.js version-specific options for the fumadocs-mdx tool, ensuring proper execution across different Node.js versions. These changes aim to streamline the installation process and improve the execution reliability of the fumadocs-mdx tool.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/www/bin/fumadocs-mdx.js (1)
6-8: Consider adding radix parameter to parseInt.The version parsing logic is correct. For completeness, consider specifying the radix parameter:
Number.parseInt(nodeVersion.slice(1).split(".")[0], 10)to ensure decimal parsing and avoid potential issues with leading zeros.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/bin/fumadocs-mdx.js(1 hunks)apps/www/package.json(1 hunks)
⏰ 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: test-e2e
🔇 Additional comments (2)
apps/www/package.json (1)
9-9: Cross-platform postinstall approach looks good.The change to use
node ./bin/fumadocs-mdx.jseffectively addresses the previous Windows compatibility concern. Node.js is cross-platform, so this will work consistently across Windows, macOS, and Linux without requiring additional dependencies like cross-env.apps/www/bin/fumadocs-mdx.js (1)
1-4: LGTM: Shebang and imports are correct.The shebang and Node.js built-in imports follow best practices.
- Introduced a new build.js script to manage Node.js version-specific options, ensuring proper execution of the build process across different Node.js versions. - Updated the postinstall script to replace fumadocs-mdx.js with a more streamlined implementation, enhancing compatibility and reliability. - Removed outdated fumadocs-mdx scripts to simplify the codebase. These changes aim to improve the build process and streamline the installation workflow.
- Updated both build.js and postinstall.js to conditionally append the `--no-experimental-webstorage` flag to NODE_OPTIONS for Node.js versions 25 and above, ensuring compatibility without overwriting existing options. - This change improves the handling of Node.js environment configurations, preventing potential conflicts with localStorage. These modifications aim to streamline the execution of scripts across different Node.js versions.
- Enhanced the postinstall.js script to include a check for the presence of pnpm, ensuring it is available in the PATH before proceeding with the installation. - This addition improves the reliability of the installation process by providing clear feedback if pnpm is not found, preventing potential runtime errors. These changes aim to streamline the installation workflow and enhance user experience.
- Modified the exit code handling in both build.js and postinstall.js to ensure a default exit code of 1 is used when no code is provided. This change enhances error handling and provides clearer feedback during script execution. These updates aim to improve the reliability of the build and installation processes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/www/bin/postinstall.js (2)
26-33: Hard-coded pnpm is acceptable for this workspace.While a past comment flagged the hard-coded
pnpmas a potential issue, it's reasonable for a postinstall script in a pnpm workspace repository. The script runs during package installation when pnpm is definitionally available.
36-38: Apply the fix for null exit codes.This issue was flagged in previous reviews but remains unresolved. When the child process is terminated by a signal, the exit code is
null, which incorrectly results in exit code 0 (success) when passed toprocess.exit().Apply the previously suggested fix:
// Forward exit code child.on("exit", (code) => { - process.exit(code); + process.exit(code ?? 1); });
🧹 Nitpick comments (4)
apps/www/bin/build.js (1)
16-16: Consider more precise flag detection.The
includes()check could theoretically match if another flag contains this substring (e.g., a hypothetical--some-no-experimental-webstorage-other-flag). While unlikely in practice, a more robust approach would split by whitespace and check exact matches.Consider this more precise check:
- if (!existingOptions.includes(flag)) { + const flags = existingOptions.split(/\s+/).filter(Boolean); + if (!flags.includes(flag)) {apps/www/bin/postinstall.js (3)
4-4: Remove unused import.The
pathmodule is imported but never used in this script.Apply this diff:
const { spawn } = require("node:child_process"); -const path = require("node:path");
10-22: NODE_OPTIONS preservation looks good; consider more precise flag detection.The code now properly preserves existing
NODE_OPTIONSand only adds the flag when needed, addressing the past review concern about overwriting user configurations.However, as with build.js, the
includes()check at line 17 could theoretically produce false positives if another flag contains this substring.For more precise matching:
// Only add the flag if it's not already present - if (!existingOptions.includes(flag)) { + const flags = existingOptions.split(/\s+/).filter(Boolean); + if (!flags.includes(flag)) {
1-43: Consider extracting shared logic to reduce duplication.The NODE_OPTIONS handling logic (lines 6-23) is duplicated between this file and
apps/www/bin/build.js. While the scripts are small and the duplication is manageable, extracting the version detection and NODE_OPTIONS setup to a shared utility function would improve maintainability.Example shared utility at
apps/www/bin/lib/node-options.js:function setupNodeOptionsForNode25() { const nodeVersion = process.version; const majorVersion = Number.parseInt(nodeVersion.slice(1).split(".")[0]); if (majorVersion >= 25) { const existingOptions = process.env.NODE_OPTIONS || ""; const flag = "--no-experimental-webstorage"; if (!existingOptions.includes(flag)) { process.env.NODE_OPTIONS = existingOptions ? `${existingOptions} ${flag}` : flag; } } } module.exports = { setupNodeOptionsForNode25 };Then both scripts could use:
const { setupNodeOptionsForNode25 } = require("./lib/node-options"); setupNodeOptionsForNode25();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/www/bin/build.js(1 hunks)apps/www/bin/postinstall.js(1 hunks)apps/www/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/www/bin/postinstall.js (1)
apps/www/bin/build.js (6)
require(3-3)nodeVersion(6-6)majorVersion(7-7)existingOptions(12-12)flag(13-13)child(28-31)
apps/www/bin/build.js (1)
apps/www/bin/postinstall.js (6)
require(3-3)nodeVersion(7-7)majorVersion(8-8)existingOptions(13-13)flag(14-14)child(26-33)
⏰ 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). (3)
- GitHub Check: test-e2e
- GitHub Check: test-typesafety
- GitHub Check: pkg-pr-new
🔇 Additional comments (7)
apps/www/bin/build.js (3)
1-7: LGTM! Version detection is correct.The shebang, imports, and major version parsing logic are appropriate and consistent with the postinstall script.
24-31: LGTM! Command spawning is properly configured.The argument forwarding and spawn configuration are correct. Using
shell: falseimproves security andstdio: "inherit"properly forwards all I/O.
38-41: LGTM! Error handling is appropriate.The error handler provides clear feedback and exits with a proper failure code.
apps/www/bin/postinstall.js (2)
6-8: LGTM! Version detection is consistent.The version parsing logic matches the pattern used in build.js and is correct.
40-43: LGTM! Error handling is correct.The error handler provides appropriate feedback and exits with a failure code.
apps/www/package.json (2)
6-6: LGTM! Build script correctly delegates to wrapper.The updated build script properly invokes the Node.js wrapper, which handles NODE_OPTIONS configuration and forwards the
next buildcommand to npx.
9-9: LGTM! Postinstall script eliminates cross-platform issues.Using the Node.js wrapper script eliminates the cross-platform shell syntax issues mentioned in previous reviews. The approach is cleaner and more maintainable than using
cross-env.
- Changed the flag from `--no-experimental-webstorage` to `--no-webstorage` in both build.js and postinstall.js for Node.js versions 25 and above. This update ensures compatibility and prevents localStorage conflicts without overwriting existing NODE_OPTIONS. These modifications aim to enhance the execution of scripts across different Node.js versions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/www/bin/build.js (1)
13-13: Verify the correct Node.js flag for disabling Web Storage.Same concern as in
postinstall.js: the flag is--no-webstorage, but ensure this is correct for Node.js 25+. Past review comments referenced--no-experimental-webstorage.
🧹 Nitpick comments (2)
apps/www/bin/postinstall.js (1)
4-4: Remove unused import.The
pathmodule is imported but never used in this file.Apply this diff:
-const path = require("node:path");apps/www/bin/build.js (1)
6-21: Consider extracting shared logic to reduce duplication.The Node.js version detection and NODE_OPTIONS handling logic is duplicated between
build.jsandpostinstall.js. Consider extracting this into a shared utility module to improve maintainability.For example, create
apps/www/bin/utils/node-env.js:function setupNodeEnv() { const nodeVersion = process.version; const majorVersion = Number.parseInt(nodeVersion.slice(1).split(".")[0]); if (majorVersion >= 25) { const existingOptions = process.env.NODE_OPTIONS || ""; const flag = "--no-webstorage"; if (!existingOptions.includes(flag)) { process.env.NODE_OPTIONS = existingOptions ? `${existingOptions} ${flag}` : flag; } } } module.exports = { setupNodeEnv };Then import and call it in both scripts:
const { setupNodeEnv } = require("./utils/node-env.js"); setupNodeEnv();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/bin/build.js(1 hunks)apps/www/bin/postinstall.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/www/bin/postinstall.js (1)
apps/www/bin/build.js (6)
require(3-3)nodeVersion(6-6)majorVersion(7-7)existingOptions(12-12)flag(13-13)child(28-31)
apps/www/bin/build.js (1)
apps/www/bin/postinstall.js (6)
require(3-3)nodeVersion(7-7)majorVersion(8-8)existingOptions(13-13)flag(14-14)child(34-41)
⏰ 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). (2)
- GitHub Check: test-e2e
- GitHub Check: test-typesafety
🔇 Additional comments (4)
apps/www/bin/postinstall.js (2)
25-41: LGTM!The pnpm verification and process spawning logic are correctly implemented. The pnpm availability check addresses the previous concern about hard-coding the package manager, and the spawn configuration properly forwards arguments and stdio.
43-51: LGTM!The exit code forwarding with
code ?? 1correctly handles signal terminations, and the error handler provides clear feedback. This properly addresses the previous concern about null exit codes.apps/www/bin/build.js (2)
24-31: LGTM!The argument extraction and process spawning logic are correct. Using
npxhere is appropriate for the build command, and the spawn configuration properly forwards arguments and stdio.
33-41: LGTM!The exit code forwarding with
code ?? 1correctly handles signal terminations, and the error handler provides clear feedback. This implementation is consistent withpostinstall.jsand properly addresses the previous concern.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/bin/build.js(1 hunks)apps/www/bin/postinstall.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/www/bin/build.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/www/bin/postinstall.js (1)
apps/www/bin/build.js (6)
require(3-3)nodeVersion(6-6)majorVersion(7-7)existingOptions(12-12)flag(13-13)child(28-31)
⏰ 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: test-e2e
🔇 Additional comments (5)
apps/www/bin/postinstall.js (5)
6-8: LGTM!Version detection logic is correct and consistent with the pattern used in
apps/www/bin/build.js.
13-21: LGTM! NODE_OPTIONS preservation is handled correctly.The logic properly preserves existing
NODE_OPTIONS, prevents duplicate flags, and only modifies the environment when necessary. This correctly addresses the concern raised in previous reviews.
25-31: LGTM! pnpm availability check is well-implemented.The verification correctly addresses the concern from previous reviews by failing fast with a clear error message if pnpm is not available, rather than producing a cryptic spawn error later.
33-41: LGTM! Process spawning is implemented correctly.The spawn configuration properly:
- Delegates to
pnpm exec fumadocs-mdx- Forwards CLI arguments via
process.argv.slice(2)- Uses
stdio: "inherit"to make output visible- Uses
shell: falsefor security
43-51: LGTM! Exit and error handling are correct.The exit handler properly addresses the concern from previous reviews by using
code ?? 1to default to exit code 1 when the child process is terminated by a signal (null). The error handler also provides a clear message and appropriate exit code.
- Eliminated the unused `path` import from the postinstall.js script to clean up the code and improve readability. This change contributes to a more efficient script without affecting functionality.
1 participant
These changes aim to improve the testing environment and streamline the installation process.
Closes #232
Summary by CodeRabbit
New Features
Documentation
Chores