-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Node process
object compatibility
#3368
Conversation
35d0628
to
37bb133
Compare
export const { pid, cwd, chdir, exit } = Deno; | ||
|
||
export function on(_event: string, _callback: Function): void { | ||
// TODO(rsp): to be implemented |
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.
please add
throw Error("unimplemented");
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.
Is it ok if I throw Error("unimplemented") for everything except "uncaughtException"? The idea is that Deno crashes on all uncaught exceptions anyway, and I want to prevent code that makes Node crash on uncaught exceptions to crash during adding a listener for uncaughtException.
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.
But it you think it's not a good idea then I can throw on everything and take care of it in a third party module, just let me know.
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.
Ok, I'm just throwing Error("unimplemented") for everything that is not actually implemented. I don't want to add any exceptions. Sorry for the confusion.
std/node/process.ts
Outdated
// TODO(rsp): currently setting env seems to be working by modifying the object | ||
// that is returnd by Deno.env(). Need to make sure that this is the final API | ||
// or Deno.env('key', 'value') is to be used in the future. | ||
export const env = Deno.env(); |
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.
Can you use a getter here? Otherwise everyone who imports this module will need to have --allow-env
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.
Ok, so you mean to give only a read access to the env? (I thought that --allow-env
was needed to read the env as well, wasn't it?)
If so then I can either return a copy of the env as it was in the moment of reading, or use a Proxy to handle getting properties and throw on setting them?
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.
Oh, I think I know what you mean :) Never mind what I said, I apparently had too little sleep today.
9517249
to
7923317
Compare
Amazing, when the tests finally passed even on Windows, now the license/cla got stuck! Not my day. |
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 - thanks!
This PR adds a partial implementation of Node-compatible process:
process.cwd()
process.chdir()
process.version
process.versions
process.platform
process.arch
process.pid
process.on
(no-op)process.argv
process.env
This is very incomplete but should be sufficient for a library that I need to port to Deno that uses some of those to sniff for Node.
If it is ok to have a partial implementation for a good start then it's better to have it here, if not then let me know and I'll put it as a third-party module on deno.land/x
ref #2644