Skip to content

Conversation

@nerdCopter
Copy link
Member

@nerdCopter nerdCopter commented Apr 30, 2024

  • works on Linux
  • OSX needs testing
  • Windows needs testing

Summary by CodeRabbit

  • Chores
    • Updated Node.js runtime version in CI/build workflow for improved compatibility
    • Pinned build tool dependency to a specific version for consistent builds

@nerdCopter nerdCopter changed the title yarn upgrade nw-builder@3.8.3 --save-dev upgrade nw-builder and node Apr 30, 2024
@nerdCopter nerdCopter changed the title upgrade nw-builder and node upgrade nw-builder 3.8.3 and node 14 Apr 30, 2024
@coderabbitai
Copy link

coderabbitai bot commented Nov 7, 2025

📝 Walkthrough

Walkthrough

The pull request updates the Node.js runtime version from 12 to 14 in the CI workflow and pins the nw-builder dependency to version 3.8.3, removing the caret constraint that previously allowed minor and patch version updates.

Changes

Cohort / File(s) Summary
CI/CD Configuration
.github/workflows/build.yml
Node.js version updated from 12 to 14 in the setup step
Dependency Management
package.json
nw-builder devDependency pinned from ^3.5.7 to 3.8.3 (fixed version constraint)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify Node.js 14 compatibility with the project's build, test, and package installation steps
  • Confirm nw-builder 3.8.3 compatibility and rationale for pinning the exact version (check for breaking changes between 3.5.7 and 3.8.3)

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely summarizes the main changes: upgrading nw-builder to 3.8.3 and Node.js to version 14, which matches the modifications shown in the raw summary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ef2498 and 43af4ef.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • .github/workflows/build.yml (1 hunks)
  • 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: Codacy Static Code Analysis
🔇 Additional comments (2)
.github/workflows/build.yml (1)

54-54: Confirm cross-platform build validation before merging.

The Node.js version bump from 12 to 14 is appropriate (12 is EOL, 14 is active LTS). However, the PR description explicitly states that OSX and Windows builds need testing. Since the CI matrix runs on ubuntu-22.04, macos-11, and windows-2022, ensure all three platforms complete successfully before merging.

Before merging, verify:

  1. Linux build: Confirm the existing Linux build passed with Node 14 (you noted this works).
  2. macOS build: Run the full workflow on macos-11 and confirm yarn gulp release --osx64 succeeds.
  3. Windows build: Run the full workflow on windows-2022 and confirm yarn gulp release --win32 --win64 succeeds.
  4. Artifact generation: Verify that .dmg, .exe, .zip, and package files are generated correctly on each platform.

If Node 14 introduces incompatibilities with nw-builder or downstream build tools (e.g., electron-installer-dmg on macOS), the errors should surface during these platform-specific runs.

package.json (1)

85-85: I have the breaking changes information. Let me gather data on security vulnerabilities and Node.js 14 compatibility for nw-builder 3.8.3.

Let me search for the specific Node version requirements for nw-builder 3.8.3.

Verify nw-builder 3.8.3 compatibility with Node.js 14 and document version pinning rationale.

No direct vulnerabilities have been found for nw-builder in Snyk's vulnerability database, so security is not a concern for version 3.8.3 itself.

However, the change from "^3.5.7" to "3.8.3" warrants clarification:

Breaking changes between 3.5.7 and 3.8.3:

  • 3.8.0: CLI options were changed to align with the API—this is the most significant and may affect build scripts
  • 3.8.2: Added support for multiple file paths, which changes how files can be passed
  • 3.8.3: Platforms argument now accepts comma-separated values (without spaces)

Compatibility concern:
Version 3.8.3 (from ~2017) is outdated, and explicit Node.js 14 compatibility has not been verified. NW.js's Node version should match the Node version on the host machine due to ABI differences in V8. Confirm whether this exact pin is necessary for Node 14 support or if it's a workaround for another constraint.

Before merging:

  1. Document why 3.8.3 was chosen over a caret range (e.g., ^3.8.3), especially if this is a known compatibility constraint
  2. Test the build pipeline with Node 14 on Windows and macOS, since this PR is marked untested on those platforms
  3. If the pin is intentional for reproducibility, consider using ~3.8.3 instead to allow patch-level security updates while maintaining stability

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

nerdCopter added a commit to nerdCopter/EmuConfigurator_nerdRepo that referenced this pull request Nov 10, 2025
- Upgrade Node.js from 12 to 14.21.3 (LTS)
- Upgrade nw-builder from 3.5.7 to 3.8.3 (fixes NW.js download)
- Add .nvmrc for nvm users
- Update yarn.lock with Node 14 compatible dependencies

Based on PR emuflight#519 which resolves NW.js download failures
@nerdCopter nerdCopter closed this Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant