-
Notifications
You must be signed in to change notification settings - Fork 479
Improve node:vm implementation to avoid unnecessary throws
#5606
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
f0ffde8 to
384122c
Compare
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.
I think you can simplify the validation of the timeout by destructuring with a default value of 1
f187b44 to
b0bba9e
Compare
|
From what I can tell the internal build job failures are due to unrelated tests failing. It is not clear whether I can do something to fix these. I tried re-running them in case they were just flakes... |
The current implementation throws "not implemented" errors on
createContextandnew Script.This is a breaking change from how unenv and Node.js work.
Now they return instances that are safe since they don't have any implementation, or actually also throw "not implemented" errors for their methods.
This change will allow us to safely remove the
node:vmpolyfill from the Cloudflare unenv preset.For reference: