Skip to content
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

feat(cli): give access to process global everywhere #25291

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Aug 29, 2024

What do we do with the TypeScript types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we do with the TypeScript types?

I think we probably want to add our own type for process and then maybe if the user imports @types/node it will start using that process global instead (this is pretty trivial for me to implement)? For now, we can just add our own type.

Copy link
Member

@dsherret dsherret Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we should merge without typescript types for now?

@Hajime-san
Copy link
Contributor

Is the intent of this implementation some kind of requirement that cannot be met by import process from 'node:process'?
Personally, I find it strange that the deno runtime has the global variable that are not compatible with browsers.
I would like to hear about any discussions that took place within the core team regarding this and their conclusions.

@bartlomieju
Copy link
Member

Is the intent of this implementation some kind of requirement that cannot be met by import process from 'node:process'? Personally, I find it strange that the deno runtime has the global variable that are not compatible with browsers. I would like to hear about any discussions that took place within the core team regarding this and their conclusions.

Deno global var is also not compatible with browsers. There's a multitude of bugs and simple mismatches that would be fixed by exposing process by default. In our testing a lot of frameworks works pretty much flawlessly in Deno barring ReferenceError: process is not defined. While we could add a significant bit of machinery to catch these errors and provide hints (in addition to lint rule), it makes everyone's lives so much easier to just have it exposed.

@Hajime-san
Copy link
Contributor

Admittedly, Deno namespaces are not compatible with browsers.
However, since there is a Deno-specific implementation under the Deno namespace, we believe that users will be able to recognize this as a Deno-specific API.
Similarly, I have often thought that if it were implemented like NodeJs.process...

I'm glad to hear that exposing global variables is intended to improve compatibility with Node.js.

I heard that Deno's Node.js compatibility works by internally determining whether or not it is in compatibility mode.
This process probably falls outside the scope of this compatibility mode.
Therefore, we believe that this is an API for compatibility only, and we do not recommend active use of it.
I believe that informing users about this point in the type definition or documentation is consistent with Deno's stance toward Node.js compatibility.

I really wanted to hear it. Thanks!

@bartlomieju
Copy link
Member

This would close #24978 (and others...)

@lucacasonato lucacasonato merged commit b333dcc into denoland:main Sep 4, 2024
17 checks passed
@lucacasonato lucacasonato deleted the process_everywhere branch September 4, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants