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

doc: document globalThis TC39 stage 3 proposal #27399

Closed
wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
Member

Document globalThis from the TC39 stage 3 proposal and doc-deprecate
global. It's been available since V8 7.0 and Node.js v11.0.0.

Float a one-line patch on top of eslint for the moment to teach it about
the new global. I'll be opening an upstream pull request in tandem with
this change.

Refs: https://github.com/tc39/proposal-global

Make the node-core/required-modules eslint rule smart enough
to recognize that `import '../common/index.mjs'` in ESM files
imports the mandatory 'common' module.
Document `globalThis` from the TC39 stage 3 proposal and doc-deprecate
`global`. It's been available since V8 7.0 and Node.js v11.0.0.

Float a one-line patch on top of eslint for the moment to teach it about
the new global. I'll be opening an upstream pull request in tandem with
this change.

Refs: https://github.com/tc39/proposal-global
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Apr 24, 2019
@bnoordhuis bnoordhuis added the doc Issues and PRs related to the documentations. label Apr 24, 2019
doc/api/globals.md Outdated Show resolved Hide resolved

<!-- type=global -->

> Stability: 0 - Deprecated: Use [`globalThis)`][] instead.
Copy link
Member

Choose a reason for hiding this comment

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

Has the project decided to deprecate global now that globalThis is available?

I think there are still cases global is useful - in particular I've seen it used when probing for an environment that is Node.js and not "universal JavaScript".

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally have never seen code that checks for the presence of global, it's always if (typeof process === 'object'), but doc-deprecating it won't break that.

The motivation for deprecation is that there should be One Way To Do It. globalThis seems strictly superior since it works in node and modern browsers.

Copy link
Member

@ljharb ljharb Apr 27, 2019

Choose a reason for hiding this comment

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

Definitely don’t deprecate global; it’s so heavily used that it isn’t web compatible to name globalThis global :-)

In other words, there are many millions of examples in GitHub code search alone of code that does this.

Copy link
Member

@joyeecheung joyeecheung Apr 27, 2019

Choose a reason for hiding this comment

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

I agree it’s not possible to runtime deprecate global at this point but this document only doc-deprecate it? We may come back revisiting it or it could just stay doc deprecated forever but this sends a message to developers that they should use globalThis instead in new code.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think the message is needed. If they want to write universal code, they either can already rely on “every single bundler” handling global transparently for them, or they can use globalThis that works in both.

I just don’t think there’s any value or need to push people in a direction here.


* {Object} The global namespace object.

## globalThis
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should document this as it's a JavaScript global.

Everything else in globals.md is a Node.js specific global that is not available in "plain" JavaScript. That is - nothing else here is defined in the ECMAScript spec (we don't list Array, String etc here for example).

Copy link
Member

Choose a reason for hiding this comment

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

well it's currently not in ecma262, it's still stage 3

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest holding off for the time being

@@ -0,0 +1,11 @@
'use strict';

require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

I am also not sure why we need a test for this as we don't have tests for other JavaScript features.

Copy link
Member Author

Choose a reason for hiding this comment

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

One reason is that it'll block this PR from accidentally getting back-ported to release branches that don't have globalThis. But really it's because I figured some extra sanity checks won't hurt.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

eslint patch and related code lgtm, not sure about the other changes here


<!-- type=global -->

> Stability: 0 - Deprecated: Use [`globalThis)`][] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also have a deprecation code filed in deprecations.md.

@Fishrock123
Copy link
Contributor

I'm unsure that deprecating global is good at this time. It is not the same as process.

cc @ljharb Who I think worked on the globalThis spec...

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

While documenting globalThis and encouraging it is great (tests are great too) i think it would be a terrible idea to deprecate global in any way whatsoever.


<!-- type=global -->

> Stability: 0 - Deprecated: Use [`globalThis)`][] instead.
Copy link
Member

@ljharb ljharb Apr 27, 2019

Choose a reason for hiding this comment

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

Definitely don’t deprecate global; it’s so heavily used that it isn’t web compatible to name globalThis global :-)

In other words, there are many millions of examples in GitHub code search alone of code that does this.

@bnoordhuis
Copy link
Member Author

I'll close this. The point of this PR is to steer people towards the standard solution rather than the node-specific one. Doc-deprecating global is an essential part of that.

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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants