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

Use core-js for getGlobals example #17337

Merged
merged 5 commits into from
Aug 25, 2022
Merged

Use core-js for getGlobals example #17337

merged 5 commits into from
Aug 25, 2022

Conversation

Josh-Cena
Copy link
Member

Summary

Motivation

Fix #10470. I hope this can reduce the confusion.

Also reverted back from const to var because this is verbatim-copied from the original code. Let me know if we still want to use const.

Supporting details

Related issues

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

@Josh-Cena Josh-Cena requested a review from a team as a code owner June 16, 2022 07:24
@Josh-Cena Josh-Cena requested review from teoli2003 and removed request for a team June 16, 2022 07:24
@github-actions github-actions bot added the Content:JS JavaScript docs label Jun 16, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
Title: globalThis
on GitHub

(this comment was updated 2022-08-25 18:32:52.159186)

@zloirock
Copy link
Contributor

zloirock commented Jun 16, 2022

@Josh-Cena
Copy link
Member Author

Should we point to core-js then? We seem to use core-js links in more places.

@zloirock
Copy link
Contributor

As you wish. I just pointed out that es6-shim is not the best option here.

@Josh-Cena
Copy link
Member Author

Yes—I'm not sure about the editors' opinions on this, so just asking :)

@teoli2003 teoli2003 requested review from wbamberg and removed request for teoli2003 June 16, 2022 09:11
@sideshowbarker sideshowbarker added the needs info Needs more information to review or act on. label Jun 22, 2022
@Josh-Cena
Copy link
Member Author

@wbamberg What do you think here? Should we change the link and the code example to core-js's, or do we just apply the current change?

@wbamberg
Copy link
Collaborator

@Josh-Cena , I'd be happy to use core-js here instead, if you want to make that change :).

@Josh-Cena Josh-Cena changed the title Point es6-shim getGlobal to the specific line Use core-js for getGlobals example Jun 30, 2022
@Josh-Cena
Copy link
Member Author

@zloirock One thing I'm unsure of: why is it necessary to check for it twice in it && it.Math == Math && it? I think I'm overlooking something.

@zloirock
Copy link
Contributor

@Josh-Cena the first it is null check - typeof null is object. The second usage of it is returned result if the previous expression is true.

@Josh-Cena
Copy link
Member Author

Josh-Cena commented Jun 30, 2022

If it's null—the check would short-circuit and return a falsy value null already, and if it's not null—neither should the second round of check be falsy, right? I was asking if there's any difference from, say, it && it.Math == Math.

Ah, sorry, I see now. Thanks!

(function () { return this; })() ||
Function('return this')();

if (typeof globalObject.setTimeout !== 'function') {
Copy link
Contributor

@zloirock zloirock Jul 2, 2022

Choose a reason for hiding this comment

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

You can check typeof setTimeout without globalObject, so I don't think that it's a good example. However, I'm not sure that it's in the scope of this PR.

Copy link
Member Author

@Josh-Cena Josh-Cena Jul 2, 2022

Choose a reason for hiding this comment

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

Let's just remove it—we are showing globalThis anyway.

I realized that by removing it we are essentially providing a polyfill😅 Do you have a better use-case in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, it's for operations with global variables that are more complex than a simple check. For example,

if (!globalObject.Promise) {
  Object.defineProperty(globalObject, 'Promise', {
    value: function Promise () {
      // your Promise implementation
    },
    enumerable: false,
    configurable: true,
    writable: true
  });
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's very useful inspiration—decided to use Intl instead of Promise so it looks more realistic.

@Josh-Cena Josh-Cena removed the needs info Needs more information to review or act on. label Jul 2, 2022
@Josh-Cena
Copy link
Member Author

Josh-Cena commented Aug 11, 2022

👋 Anyone to review this? @wbamberg?

@Josh-Cena
Copy link
Member Author

Trying to summon @wbamberg again since this one is quite small

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Sorry to be slow @Josh-Cena .

@Josh-Cena Josh-Cena force-pushed the fix-getglobal branch 3 times, most recently from 019f599 to 3fe256a Compare August 25, 2022 03:24
check(typeof global === 'object' && global) ||
// This returns undefined when running in strict mode
(function () { return this; })() ||
Function('return this')();
Copy link
Contributor

@zloirock zloirock Aug 25, 2022

Choose a reason for hiding this comment

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

Now, without globalThis, it will not work in cases of ES-only environments (no one of the variables above) in modules context (IIFE this is undefined), where for security reasons disabled evaluation via eval / Function. It's not a big problem to create such an environment, for example, with NodeJS vm.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right—the purpose of this example is to illustrate what happens without globalThis. I did dig into the history of internals/global.js and it was something like this a long time ago: https://github.com/zloirock/core-js/blob/b4e8c0b09bbe23db8d8784df03ef0b8f3ff04170/packages/core-js/internals/global.js

Copy link
Contributor

Choose a reason for hiding this comment

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

And after that, someone wrote me an issue about such an environment.

Someone could try to use the code from this example as a universal way of getting the global object and theoretically, it could fail in this specific and rare case. So I think changing "prior to globalThis ..." to something else could be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing I've modified from the core-JS source is removal of the globalThis mention. In a runtime without globalThis, is there anything better than this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that someone could try this code everywhere as something universal - in environments without globalThis and in environments with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh well, it was kind of the same code before—let's hope no one uses it as a polyfill (they shouldn't!) If you want to add a warning box making it explicit, feel free to.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thanks for your patience @Josh-Cena !

@wbamberg wbamberg merged commit 0bba6dd into mdn:main Aug 25, 2022
@Josh-Cena Josh-Cena deleted the fix-getglobal branch August 25, 2022 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:JS JavaScript docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with "globalThis": ("the global across environments" content is outdated))
4 participants