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

lib: expose a global node proxy in eval script and REPL #42114

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 24, 2022

This global is meant to be a convenient shortcut to access core modules.

  • figure out REPL autocompletion for the proxy.
  • fix Unexpected global(s) found: node test failures.

This global is meant to be a convenient shortcut to access core modules.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. repl Issues and PRs related to the REPL subsystem. labels Feb 24, 2022
@devsnek
Copy link
Member

devsnek commented Feb 24, 2022

what's the reasoning for this? all the core modules are already available as globals in the repl

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 24, 2022

what's the reasoning for this? all the core modules are already available as globals in the repl

The idea would be to deprecate that. The problem with having Node.js specific globals is that it clashes with other globals (e.g. crypto references the Web Crypto API in other JS runtimes).

lib/internal/process/execution.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
@aduh95 aduh95 mentioned this pull request Feb 26, 2022
24 tasks
@tniessen
Copy link
Member

I haven't given this much thought yet, but I don't know if I am a fan of adding another global as the solution to "we added globals that are now a problem."

@Slayer95
Copy link

Slayer95 commented Mar 2, 2022

The issue with crypto should be studied from the lens of scripts and libraries maintainers.

Currently, code that is to be consumed by both client and server must feature-test the content of the crypto global to know what it is about.

After several Node.js major versions are released, the old versions will keep being used in the wild, and so library maintainers will still need to feature-test the crypto global to figure out how to work with or without it.

So on one hand the end-user benefits are non-existent. And on the other hand, the deprecation of fs, path and other globals in the REPL will noticeably reduce its usability for quick scripts.

@antsmartian
Copy link
Contributor

@aduh95 ping. Are we planning to work on this to get it to a closure?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 17, 2023

Not really. The idea implemented in this PR was to solve the clash for crypto name in REPL and node --eval context between node:crypto and globalThis.crypto, but we went another way and I haven't seen any report complaining with the way it's implemented.
Closing this as I'm not working on it, but I'm curious to hear if you have thoughts on whether we should reconsider it and why.

@aduh95 aduh95 closed this Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants