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

chore: edit globalThis.js to first check for globalThis #1449

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

katelynsills
Copy link
Contributor

@katelynsills katelynsills commented Feb 5, 2021

Corresponding issue (if exists):

N/A, but can create if that would be helpful.

What would you like to add/fix?

Now that globalThis is generally available on all the latest platforms, globalThis.js can be improved by first checking for the presence of globalThis.

My motivation: At Agoric, we have a secure version of JavaScript called SES. Under SES, globalThis exists, but window, self, and Function('return this')() are all undefined for security reasons. So, jss was failing for us, but by including globalThis, it will safely succeed even under our security requirements.

Todo

  • Add test that verifies the modified behavior

I would be happy to add a test, but I wasn't sure what to test and where to put it. It looks like there are no unit tests for globalThis. I'm testing locally that jss works for me under SES with this change.

  • Add documentation if it changes public API

@kof
Copy link
Member

kof commented Feb 5, 2021

What a great PR. I think testing this is not necessary, because to test this we would have to simulate that environment and we would be mostly testing the simulation, since there isn't much logic in there.

I think what we need is to add an inline comment with exactly what you just said in the description, so that it's clear why it exists and we can merge it.

@kof
Copy link
Member

kof commented Feb 5, 2021

I just looked into the current version of core-js of this, https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/global.js

@katelynsills
Copy link
Contributor Author

katelynsills commented Feb 5, 2021

Ok, great! Would adding the following comment work?

Now that `globalThis` is available on most platforms (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#browser_compatibility) we check for `globalThis` first.

@kof
Copy link
Member

kof commented Feb 6, 2021

That and also your use case with that execution environment

@katelynsills
Copy link
Contributor Author

That and also your use case with that execution environment

That comment has been added!

@kof kof merged commit a4f58de into cssinjs:master Feb 9, 2021
@kof
Copy link
Member

kof commented Feb 9, 2021

merged, thank you, will be released with the next release at some point. I aggregate a bit.

@kof
Copy link
Member

kof commented Mar 14, 2021

Released in v10.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants