Improve Node.js compatibility for process, fs, http, events, and navigator#6575
Improve Node.js compatibility for process, fs, http, events, and navigator#6575
Conversation
|
UnknownError: ProviderInitError |
|
@danlapid Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
The generated output of |
38e9619 to
29fe1d8
Compare
29fe1d8 to
275eb25
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
|
/bonk review |
There was a problem hiding this comment.
Review Summary
Overall this PR is a clean batch of additive Node.js compat stubs. A few items worth considering:
- [LOW]
navigator.platform: Added unconditionally whilelanguage/languagesare gated behindenableNavigatorLanguage. Confirm this is intentional — the spec saysplatformis deprecated but returning""is the recommended deprecation path, not"Linux". - [LOW]
process.config.variables.node_shared_merve: Likely a typo — this field doesn't exist in Node.js's actualprocess.config. - [LOW]
export { constants }placement ininternal_fs_promises.ts: Interleaved between import statements — technically valid but unusual; consider grouping with other exports. - [LOW]
process.configblob: ~100 lines of hardcoded values that may silently drift from Node.js over time. - [INFO] The CodSpeed regression on
simpleStringBody[Response](-25%) is flagged as "different runtime environments" — likely CI noise unrelated to this PR. - [INFO] The PR description mentions "events: Export captureRejections as named export" but this was already exported via
export *inevents.ts. No issue, just a documentation inaccuracy.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted successfully on PR #6575. Here's a summary of the findings: 7 inline comments across the changed files:
|
petebacondarwin
left a comment
There was a problem hiding this comment.
Approved for typings changes on behalf of Wrangler team
…gator Add missing properties and constants identified by the node_compat matrix to close gaps between workerd and Node.js. All changes are additive — they either populate previously missing properties with Node.js-compatible values or fix export wiring so existing values are reachable. I believe no compat flag should be needed because: - New properties on existing objects (process.config.variables, stdio socket-like properties, EventEmitter.domain, globalAgent options fields) cannot break existing code since the keys didn't exist before, and all values match Node.js defaults. - process.config previously had empty target_defaults/variables objects, so code reading specific config keys got undefined before and gets the correct value now. Code checking for emptiness is unlikely since the empty stub was useless. - fs.promises.constants was missing due to an export wiring issue — the constants existed in the internal module but weren't re-exported through the promises entry point. - The HTTP/HTTPS Agent constructor changes align with Node.js's actual initialization order (setting defaultPort/protocol/path/proxyEnv on this.options), which fixes missing keys on agent.options without changing any observable behavior for existing Agent usage. Specific changes: - process.config: Populate with representative Node.js build config stubs - process.stdout/stderr/stdin: Add socket-like properties (bytesRead, connecting, readyState, writable*, readable*, etc.) - fs.promises.constants: Re-export constants from internal_fs_promises - http/https Agent: Match Node.js constructor flow for options.path, options.proxyEnv, options.defaultPort, options.protocol; add agentKeepAliveTimeoutBuffer and maxCachedSessions - EventEmitter: Set domain = null in init() matching Node.js - navigator.platform: Add read-only property returning "Linux" - module.prototype.isPreloading: Add boolean stub - events: Export captureRejections as named export
df658b4 to
95d305f
Compare
Co-authored-by: Guy Bedford <guybedford@gmail.com> Co-authored-by: Dan Lapid <dan.lapid@gmail.com>
95d305f to
83053d0
Compare
Add missing properties and constants identified by the node_compat matrix to close gaps between workerd and Node.js. All changes are additive — they either populate previously missing properties with Node.js-compatible values or fix export wiring so existing values are reachable. I believe no compat flag should be needed because:
Specific changes: