Skip to content

doc: mark global object as legacy - #47819

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
mertcanaltin:dev-47784
May 9, 2023
Merged

doc: mark global object as legacy#47819
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
mertcanaltin:dev-47784

Conversation

@mertcanaltin

@mertcanaltinmertcanaltin commented May 2, 2023•
edited
Loading

Copy link
Copy Markdown
Member

doc: mark global object as legacy

Fixes: #47784

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

benjamingr commented May 3, 2023

Copy link
Copy Markdown
Member

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

Considered legacy by whom?

@mertcanaltin

mertcanaltin commented May 3, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

dnalborczyk commented May 3, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@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

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

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

mertcanaltin commented May 6, 2023

Copy link
Copy Markdown
MemberAuthor

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 tniessenMay 6, 2023 16:35
@jimmywarting

jimmywarting commented May 6, 2023•
edited
Loading

Copy link
Copy Markdown

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

mertcanaltin commented May 6, 2023

Copy link
Copy Markdown
MemberAuthor

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

@mertcanaltinmertcanaltin changed the title doc: update documentation for deprecated global objectdoc: update documentation for heritage global objectMay 6, 2023
targos
targos reviewed May 7, 2023
View reviewed changes
Comment threaddoc/api/globals.md Outdated
@mertcanaltin
mertcanaltin requested a review from targosMay 7, 2023 11:52
tniessen
tniessen reviewed May 7, 2023
View reviewed changes
Comment threaddoc/api/globals.md Outdated
@mertcanaltin
mertcanaltin requested a review from tniessenMay 7, 2023 12:14
tniessen
tniessen approved these changes May 7, 2023
View reviewed changes
Comment threaddoc/api/globals.md Outdated
@tniessentniessen 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

tniessen commented May 7, 2023

Copy link
Copy Markdown
Member

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.)

@mertcanaltinmertcanaltin changed the title doc: update documentation for heritage global objectdoc: mark global object as legacyMay 7, 2023
@mertcanaltin
mertcanaltin requested a review from tniessenMay 7, 2023 14:12
@tniessen

tniessen commented May 9, 2023

Copy link
Copy Markdown
Member

cc @nodejs/tsc for visibility

mcollina
mcollina approved these changes May 9, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@nodejs-github-botnodejs-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:mainMay 9, 2023
@nodejs-github-bot

nodejs-github-bot commented May 9, 2023

Copy link
Copy Markdown
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: #47819Fixes: #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>
@targostargos 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: #47819Fixes: #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#47819Fixes: 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>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
+1 more reviewer
@aymen94aymen94aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues 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