-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(unenv-preset): add native node:repl module support #12007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 9ef23b8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
14425eb to
8cad740
Compare
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
| const repl = await import("node:repl"); | ||
|
|
||
| // Common exports (both unenv stub and native workerd) | ||
| assertTypeOfProperties(repl, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could factor code
for (const target of [repl, repl.default]) {
assertTypeOfProperties(target, { ... });
}we could also do that across the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that in a general refactor PR afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin PR requested by @petebacondarwin
Adds support for the native
node:replmodule from workerd when theenable_nodejs_repl_moduleandexperimentalcompatibility flags are enabled. This follows the same pattern as other experimental modules (inspector, sqlite, dgram, stream_wrap).The native code appears in workerd as of https://github.com/cloudflare/workerd/blob/main/src/node/repl.ts. It is currently experimental with no default enable date.
Implementation Comparison
writerERR_METHOD_NOT_IMPLEMENTEDstartERR_METHOD_NOT_IMPLEMENTEDRecoverableREPLServerREPL_MODE_SLOPPYREPL_MODE_STRICTbuiltinModules_builtinLibsBreaking change analysis: There are no breaking changes when moving from unenv to workerd. Both implementations throw errors for the main functions (
writer,start,REPLServer,Recoverable). The only difference is the error type/message - workerd throwsERR_METHOD_NOT_IMPLEMENTED(Node.js style) while unenv throws a generic "not implemented" error. The workerd implementation is moving closer to Node.js by using proper Node.js error codes.Updates since last revision
assertTypeOfPropertiesfor symbol assertions (REPL_MODE_SLOPPY,REPL_MODE_STRICT) per review feedbackHuman Review Checklist
getReplOverrides()function follows the same pattern as other experimental modules/not implemented|ERR_METHOD_NOT_IMPLEMENTED/correctly handles both error typesLink to Devin run