Skip to content

Conversation

@chocolateboy
Copy link

@chocolateboy chocolateboy commented Jul 31, 2021

This PR:

  • replaces self with globalThis
  • makes detection of CommonJS environments more robust

globalThis is the official way to refer to the global scope and is supported by almost all environments:

engine self globalThis
Node.js
QuickJS
SpiderMonkey
Modern browsers
Deno
IE11

The notable exception is IE11, which will need to use a polyfill. If IE11 support was previously promised or assumed, then this would be a breaking change.

@davidchambers
Copy link
Member

Thanks for the pull request, @chocolateboy! I pushed a few tweaks to your branch. Are you happy with them?

@chocolateboy
Copy link
Author

Yes, LGTM, thanks!

- replace `self` with `globalThis`
- make module detection more robust

Co-authored-by: David Chambers <[email protected]>
module.exports = mapping;
} else {
self.FantasyLand = mapping;
globalThis.FantasyLand = mapping;

Choose a reason for hiding this comment

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

Wouldn't be safer to use something like (globalThis || self)? Just in case...

Copy link
Member

Choose a reason for hiding this comment

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

If globalThis does not exist in the environment, globalThis || self throws a ReferenceError. We would need to use typeof globalThis to first check for the presence of globalThis. Is this worthwhile? I'm not sure.

Choose a reason for hiding this comment

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

Yeah, you are right. I thought short-circuiting would not throw.

Copy link

@diasbruno diasbruno Nov 10, 2024

Choose a reason for hiding this comment

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

It'd be worth if it needs to be backward-compatible. But it's hard to find a reliable - and nice - way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants