fix(sdk): node SDK cross-platform compatibility #1000
Conversation
📝 Walkthrough""" WalkthroughThe changes introduce platform-specific handling for standard input file descriptor flags on macOS and Linux using the The version string for the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant OS
participant ffi-rs
participant fs
App->>OS: Detect platform (process.platform)
alt Platform is macOS or Linux
App->>ffi-rs: Dynamically import ffi-rs
App->>ffi-rs: Open libc and define fcntl
App->>ffi-rs: Get stdin flags, clear O_NONBLOCK, set new flags
ffi-rs-->>App: Return result (error if non-zero)
end
App->>fs: Read from process.stdin.fd using fs.readSync
fs-->>App: Return response data
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/typegraph/specs/codegen/rpc/typescript/client.ts (1)
93-93: Remove unnecessary fallback operator.
fs.readSyncthrows on error and always returns a number of bytes read, so the?? 0fallback is redundant.- bytesRead = fs.readSync(process.stdin.fd, buffer) ?? 0; + bytesRead = fs.readSync(process.stdin.fd, buffer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/typegraph/specs/codegen/rpc/typescript/client.ts(3 hunks)
🔇 Additional comments (2)
src/typegraph/specs/codegen/rpc/typescript/client.ts (2)
5-5: Importingprocessmodule is appropriate.
No issues found; this ESM-safe import pattern is good practice in modern Node.js.
14-14: Destructuring theplatformproperty is straightforward.
The usage is clear and consistent.
luckasRanarison
force-pushed
the
met-876-fix-node-sdk-compatibility
branch
from
b365726 to
230d33b
Compare
luckasRanarison
force-pushed
the
met-876-fix-node-sdk-compatibility
branch
from
2f80aa3 to
6360018
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1000 +/- ##
=======================================
Coverage 81.15% 81.15%
=======================================
Files 143 143
Lines 18038 18041 +3
Branches 1967 1966 -1
=======================================
+ Hits 14638 14641 +3
Misses 3382 3382
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@luckasRanarison does it change a configuration OS wide? your PR is awesome and may also be reflected in the com to explain this part of the config (next pr?) |
|
No, it's just scoped to the current process standard input, Deno does it behind the scenes. But I think it's not really needed to bring back the flag because that's our desired behavior. |
| }, | ||
| }); | ||
|
|
||
| const flags = fcntl([process.stdin.fd, F_GETFL, 0]); |
There was a problem hiding this comment.
Worth to mention that stdin/out can be shared by the parent/child processes. So, wouldn't changing the blocking mode also affect meta or something that runs meta? It might break some assumptions. The default is blocking after all.. even on linux or does the big guys like deno do the same?
There was a problem hiding this comment.
Indeed, if the stdio is inherited this also affects the parent's io behavior but it's completely safe because typegraph script processes are spawned with piped stdio. Also just a correction, it's async by default...
There was a problem hiding this comment.
piped stdio
Got it.
it's async by default...
Ubuntu 24.04
There was a problem hiding this comment.
Ubuntu 24.04
Yes
There was a problem hiding this comment.
wait. non block flag 0 is async?
There was a problem hiding this comment.
const result = fcntl([process.stdin.fd, F_SETFL, flags & ~O_NONBLOCK]);
would no op on linux (if we ignore syscalls)
There was a problem hiding this comment.
Same thing on mac, so wouldn't it just no op in all cases?
There was a problem hiding this comment.
@luckasRanarison ah, I think I get it.
- It no ops on the blocking flag (if already blocking).
2. But it clears the over flags => so that made it work?
There was a problem hiding this comment.
Sorry for the spam but I think I managed to narrow down the behavior out of curiosity.
For future references, it is platform + runtime specific only behavior (this is why I got no ops in all cases on Python3 even on MAC like the screenshots above).
- flag is 0 on (Node, Linux) and (Deno, Linux), this is why it worked without any changes.
- flag is 0 on (Deno, MAC) too, this is why it worked without any changes (and also (Python, MAC)).
- flag is 4 on (Node, MAC), and as such it does not no op but do reset it but...
.. weirdly enough.. even though flag is 4 on (Node, MAC), uncommenting the clear op const result = fcntl([process.stdin.fd, F_SETFL, flags /*& ~O_NONBLOCK*/]) which should be no op (so should EAGAIN) makes it work and removing this line makes it EAGAIN. That suggests either a bug on Node side or that what that was retrieved was actually not the actual current default state on MAC.
Migration notes
Summary by CodeRabbit