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

flag/deprecate global - recommend globalThis #47784

Closed
jimmywarting opened this issue Apr 29, 2023 · 9 comments · Fixed by #47819
Closed

flag/deprecate global - recommend globalThis #47784

jimmywarting opened this issue Apr 29, 2023 · 9 comments · Fixed by #47819
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@jimmywarting
Copy link

jimmywarting commented Apr 29, 2023

Affected URL(s)

https://nodejs.org/dist/latest-v20.x/docs/api/globals.html#global

Description of the problem

global should really not be used anymore... it's too nodejs specific. and should be marked as legacy
globalThis works everywhere and is the "preferred" way for cross compatibility

@jimmywarting jimmywarting added the doc Issues and PRs related to the documentations. label Apr 29, 2023
@bnoordhuis
Copy link
Member

Pull request welcome.

  • doc deprecation: ok
  • runtime warning: not ok (for now but maybe never)

@jimmywarting
Copy link
Author

wasn't really counting on making runtime warning and having a getter method that warns ppl.
Just want it to land some @deprecated - use globalThis instead type definition somewhere.

@anonrig
Copy link
Member

anonrig commented May 1, 2023

I'm ok with doc deprecation as well.

@KhafraDev KhafraDev added the good first issue Issues that are suitable for first-time contributors. label May 1, 2023
@AnandShiva
Copy link

I am interested on fixing this issue. Should the doc consider Stability index also, and mark that as
Stability: 3 - Legacy ? @jimmywarting .

@jimmywarting
Copy link
Author

Should the doc consider Stability index also, and mark that as Stability: 3 - Legacy ?

better to ask someone else.

@Mesteery
Copy link
Contributor

Mesteery commented May 1, 2023

I am interested on fixing this issue. Should the doc consider Stability index also, and mark that as Stability: 3 - Legacy ?

I think you can write something like this Stability: 0 - Deprecated: Use `globalThis` instead.

@tniessen
Copy link
Member

tniessen commented May 6, 2023

Legacy seems more appropriate than a deprecation given that it isn't causing any problems and the fact that we might never be able to remove global. (Besides, I'm not aware of a good reason for trying to do so.)

@jimmywarting
Copy link
Author

@tniessen a doc deprecation dose not mean that we would remove it or dispatch some runtime warning for using it.
we have other deprecated things in NodeJS that will never go away b/c they are exposed for public use.

As other have put it

@bnoordhuis

  • doc deprecation: ok
  • runtime warning: not ok (for now but maybe never)

@anonrig
I'm ok with doc deprecation as well.

@Mesteery
I think you can write something like this Stability: 0 - Deprecated: Use `globalThis` instead.

I'm okey with doc deprecation as well.

@tniessen
Copy link
Member

tniessen commented May 6, 2023

a doc deprecation dose not mean that we would remove it or dispatch some runtime warning for using it.
we have other deprecated things in NodeJS that will never go away b/c they are exposed for public use.

@jimmywarting I am familiar with our history of deprecations, which is how I know that some doc-only deprecations have had virtually zero benefits while causing a significant amount of friction.

But ignoring the past, I based my opinion on the Node.js documentation, which says:

Deprecated: The feature may emit warnings. Backward compatibility is not guaranteed.

Legacy: Although this feature is unlikely to be removed and is still covered by semantic versioning guarantees, it is no longer actively maintained, and other alternatives are available.

Based on these definitions, I personally believe that legacy better matches your concerns related to global. Whether @bnoordhuis, @anonrig, and @Mesteery find a doc-only deprecation acceptable is unrelated to my comment.

nodejs-github-bot pushed a commit that referenced this issue May 9, 2023
PR-URL: #47819
Fixes: #47784
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue May 12, 2023
PR-URL: #47819
Fixes: #47784
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
PR-URL: #47819
Fixes: #47784
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
PR-URL: nodejs#47819
Fixes: nodejs#47784
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants