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.

Sign up for GitHub

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

Jump to bottom

doc: mark global object as legacy #47819

Merged
merged 1 commit into from
May 9, 2023
Merged

doc: mark global object as legacy #47819

merged 1 commit into from
May 9, 2023

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented May 2, 2023 •
edited

doc: mark global object as legacy

Fixes: #47784

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 2, 2023
@benjamingr
Copy link
Member

The global object is Node.js specific and considered legacy.

Considered legacy by whom?

@mertcanaltin
Copy link
Member Author

mertcanaltin commented May 3, 2023 •
edited

The global object is Node.js specific and considered legacy.

Considered legacy by whom?

I tried to do it based on the comments in the issue, I may be lacking in this regard, sorry @benjamingr
we thought it was a legacy and it would be good if we suggested globalThis as a suggestion 🤔

@dnalborczyk
Copy link
Contributor

dnalborczyk commented May 3, 2023 •
edited

@benjamingr I don't know if legacy is the right therm to use, but in order to write cross-platform code, one should likely not use global anymore, and instead use globalThis.

the original proposal tried to use global, but ran into webcompat problems: https://github.com/tc39/proposal-global and subsequently renamed to globalThis.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#description

I think it's good to discourage developers from using it and a doc deprecation is a good thing.

tniessen
tniessen reviewed May 6, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

This is not a formal deprecation. As Ben said in #47784, we might never be able to warn about usage at runtime yet alone remove it. I am not convinced that formally deprecating this API helps.

We could mark it as legacy. Or we could just add a sentence saying that new applications should consider using globalThis for improved compatibility with other runtimes.

@mertcanaltin
Copy link
Member Author

This is not a formal deprecation. As Ben said in #47784, we might never be able to warn about usage at runtime yet alone remove it. I am not convinced that formally deprecating this API helps.

We could mark it as legacy. Or we could just add a sentence saying that new applications should consider using globalThis for improved compatibility with other runtimes.

yes i will update it that way if appropriate

@mertcanaltin mertcanaltin requested a review from tniessen May 6, 2023 16:35
@jimmywarting
Copy link

jimmywarting commented May 6, 2023 •
edited

I was thinking more like how all the other docs do it, take url parser for instant.

> Stability: 3 - Legacy: Use the WHATWG URL API instead.

So maybe

> Stability: 3 - Legacy. use globalThis instead.

so that it gets a little legacy label too like this
image

I also wouldn't mind if it where deprecated also.

@mertcanaltin
Copy link
Member Author

I was thinking more like how all the other docs do it, take url parser for instant.

> Stability: 3 - Legacy: Use the WHATWG URL API instead.

So maybe

Stability: 3 - Legacy. use globalThis instead.

so that it gets a little legacy label too like this image

I also wouldn't mind if it where deprecated also.

thanks for your suggestion i made an update

@mertcanaltin mertcanaltin changed the title doc: update documentation for deprecated global object doc: update documentation for heritage global object May 6, 2023
targos
targos reviewed May 7, 2023
View reviewed changes
doc/api/globals.md Outdated Show resolved Hide resolved
@mertcanaltin mertcanaltin requested a review from targos May 7, 2023 11:52
tniessen
tniessen reviewed May 7, 2023
View reviewed changes
doc/api/globals.md Outdated Show resolved Hide resolved
@mertcanaltin mertcanaltin requested a review from tniessen May 7, 2023 12:14
tniessen
tniessen approved these changes May 7, 2023
View reviewed changes
doc/api/globals.md Outdated Show resolved Hide resolved
@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2023
@tniessen
Copy link
Member

tniessen commented May 7, 2023

Optional nit: the commit message is rather inaccurate. Consider changing the first commit message to something like this:

doc: mark global object as legacy

Fixes: https://github.com/nodejs/node/issues/47784

(Or ping me if you'd like me to do so.)

@mertcanaltin mertcanaltin changed the title doc: update documentation for heritage global object doc: mark global object as legacy May 7, 2023
@mertcanaltin mertcanaltin requested a review from tniessen May 7, 2023 14:12
@tniessen
Copy link
Member

tniessen commented May 9, 2023

cc @nodejs/tsc for visibility

mcollina
mcollina approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@nodejs-github-bot nodejs-github-bot merged commit 33231b0 into nodejs:main May 9, 2023
17 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 33231b0

targos pushed a commit that referenced this pull request May 12, 2023
@mertcanaltin @targos
doc: mark global object as legacy
7ed99e8 
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 targos mentioned this pull request May 15, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@mertcanaltin @danielleadams
doc: mark global object as legacy
195eef6 
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 pull request Jul 6, 2023
@mertcanaltin @MoLow
doc: mark global object as legacy
b80e006 
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>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@aymen94 aymen94 aymen94 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flag/deprecate global - recommend globalThis
10 participants
@mertcanaltin @benjamingr @dnalborczyk @jimmywarting @tniessen @nodejs-github-bot @mcollina @targos @cjihrig @aymen94