Conversation
There was a problem hiding this comment.
Other comments (2)
- WORKSPACE (226-226) The call to `extract_versions(node_version = NODE_VERSION)` depends on `NODE_VERSION` being defined above. Consider adding a comment to clarify this dependency, to prevent accidental reordering that could break the build.
- src/workerd/api/node/util.h (165-165) The removal of processPlatform and getProcessPlatform() eliminates a utility for determining the platform string in a cross-platform way. If this functionality is still needed for Node.js compatibility, consider reimplementing it or providing it in another module.
1 file skipped due to size limits:
- src/workerd/api/node/tests/util-nodejs-test.js
💡 To request another review, post a new comment with "/windsurf-review".
b2f4b19 to
a904d28
Compare
FYI I think that was the right thing to do even if we broke other (older) libs along the way, i..e cloudflare/workers-sdk#9352 /cc @mikenomitch @anonrig |
|
@vicb am I understanding correctly that your shim caused this to pass in your workers apps, therefore you've effectively already tested "turning on" these feature gates? Are there more cases to worry about that rely on other compat features being available (e.g. should we hold back on this for FS to fully land?). The feedback is definitely good to know in terms of the risk assessment of this PR as well. |
05cbd0e to
b1c3940
Compare
|
process.platform - Returns OS platform (darwin/linux/win32) - [Dan]: should likely always be linux, users dont need to know which OS their workers run on |
|
With the version, For the platform - we were already supporting in local workerd providing the native platform information. I've removed this, but in theory that is a breaking change for non edge worker users. I've reduced the version list to just node now, with a manually maintained version. I've also updated the argv to use |
835ac7d to
467c047
Compare
|
The generated output of |
We have process.versions.node define in OpenNext for probably a couple months. It landed in unenv more recently in Wrangler 4.16. |
317e175 to
565d2d4
Compare
9b88267 to
374159e
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
|
This PR is ready for final review, with the immediate follow-ups remaining being |
This comment was marked as off-topic.
This comment was marked as off-topic.
npaun
left a comment
There was a problem hiding this comment.
process.platform implementation LGTM
This reverts commit 3edffd2.
…ures Revert "NodeJS Compat: process features (#4271)"
This reverts commit ae2c282.
This reverts commit ae2c282.
This reverts commit ae2c282.
This reverts commit ae2c282.
This reverts commit ae2c282.
…"""" This reverts commit e940848.
…"""" This reverts commit e940848.
…"""" This reverts commit e940848.
…"""" This reverts commit e940848.
…"""" This reverts commit e940848.
…"""" This reverts commit e940848.
Here's an initial stab at better process support.
For
process.versions, we just include the node version maintained through a header file that will need to be manually updated.I included the version information for node, v8, icu, ada, brotli, simdutf, sqlite, nbytes, ncrypto as part of the Bazel build process. This exposes internal implementation details which might be considered a security risk. This is also what all Node-like platforms do.We can discuss restricting this list down to just node and v8 or even just node.process.versions.nodeis a common compat check though so it's worth bearing that one in mind at least. Happy to remove, not tied to the implementation at all, would value feedback here.Included Features
Properties:
process.release - Node.js release metadata, hardcoded to Node.js default (this is what Bun & Deno do, open to leaving this out as well)(removed)Methods:
Events
warningonprocess.emitWarningrejectionHandledper the current global eventunhandledRejectionper the current global eventFollow-on Features
The following features will be implemented as follow-ons:
Excluded Features
The following features are still entirely omitted and only exported as
undefined: