fix Windows node-pty packaging#1
Conversation
Promote the Windows node-pty binary to a top-level optional dependency and add a packaging regression check so Windows installers ship the PTY runtime FastAgents needs at startup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThese changes add packaging validation for Windows distributions by introducing a test script that verifies the optional Windows-specific dependency ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/packaging-node-pty.test.mjs`:
- Line 6: The test uses import.meta.dirname to build appRoot which fails on Node
20.0–20.10; replace that usage by deriving the directory from import.meta.url
using fileURLToPath and path.dirname (i.e., compute const appRoot =
path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..')) or
alternatively raise the engines constraint to >=20.11.0; update the code around
the appRoot declaration and add the required import of fileURLToPath from 'url'
if not present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc7a63d-3b9f-4092-9e00-3a86cc1d4238
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonscripts/packaging-node-pty.test.mjs
| import fs from 'node:fs' | ||
| import path from 'node:path' | ||
|
|
||
| const appRoot = path.resolve(import.meta.dirname, '..') |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
From official Node.js documentation/changelog: in which Node.js versions is import.meta.dirname supported, and is it available in all Node 20.x releases?
💡 Result:
import.meta.dirname is supported in Node.js starting from v20.11.0 and v21.2.0, and in all subsequent versions including all later 20.x releases (20.11.0+). It is not available in Node 20.x releases prior to 20.11.0. From the official Node.js documentation (Modules: ECMAScript modules), it states: "Added in: v21.2.0, v20.11.0". The feature was added via PR #48740 merged to main and backported to v20.x via PR #50502. Multiple secondary sources (Stack Overflow, Sonar, Reddit, Stackademic) confirm introduction in 20.11.0. Node 20 LTS changelog lists 20.11.0 as a release, and documentation for latest v20.x and v25.x confirms presence. No sources indicate availability before 20.11.0 in 20.x.
Citations:
- 1: https://nodejs.org/api/esm.html
- 2: esm: add import.meta.dirname and import.meta.filename nodejs/node#48740
- 3: nodejs/node@eac9cc5fcb
- 4: https://stackoverflow.com/questions/46745014/alternative-for-dirname-in-node-js-when-using-es6-modules
- 5: https://www.sonarsource.com/blog/dirname-node-js-es-modules
- 6: https://www.reddit.com/r/node/comments/195lpbd/nodejs_2011_release_adds_importmetadirname_and/
- 7: esm: backport dirname and filename to v20 nodejs/node#50502
- 8: Missing
import.meta.dirname/import.meta.filenamesupport for node mdn/content#38335 - 9: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V20.md
- 10: https://nodejs.org/en/blog/release/v20.11.0
import.meta.dirname is not supported in older Node 20.x versions
Line 6 uses import.meta.dirname, which is only available in Node.js v20.11.0 and later. Since your project declares node >=20, versions 20.0–20.10 will fail at runtime. Use path.dirname(fileURLToPath(import.meta.url)) instead to support the full declared range, or raise the engine floor to >=20.11.0.
Compatibility-safe change
import test from 'node:test'
import assert from 'node:assert/strict'
import fs from 'node:fs'
import path from 'node:path'
+import { fileURLToPath } from 'node:url'
-const appRoot = path.resolve(import.meta.dirname, '..')
+const currentDir = path.dirname(fileURLToPath(import.meta.url))
+const appRoot = path.resolve(currentDir, '..')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/packaging-node-pty.test.mjs` at line 6, The test uses
import.meta.dirname to build appRoot which fails on Node 20.0–20.10; replace
that usage by deriving the directory from import.meta.url using fileURLToPath
and path.dirname (i.e., compute const appRoot =
path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..')) or
alternatively raise the engines constraint to >=20.11.0; update the code around
the appRoot declaration and add the required import of fileURLToPath from 'url'
if not present.
|
感谢你的 PR。 这个 Windows node-pty 打包问题已经在当前 main 里通过 我先不合并这个 PR，原因是：
所以这个 PR 我先关闭。后续如果要加 Windows 打包回归测试，我会基于当前 |
Summary
@lydell/node-pty-win32-x64to a top-level optional dependency so electron-builder includes it in Windows packagespnpm run dist:winTest plan
pnpm testpnpm run dist:windist/win-unpacked/resources/app.asar.unpacked/node_modules/@lydell/node-pty-win32-x64/package.jsonexists🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores