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

feat: make it strict mode & CSP compatible if possible #396

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Jack-Works
Copy link

@Jack-Works Jack-Works commented May 28, 2020

Use well-known global names if possible

@Jack-Works
Copy link
Author

@benjamn hi, can you review this? this is a small PR

@Jack-Works
Copy link
Author

@benjamn hi, can you review this? this is a small PR

@benjamn
Copy link
Collaborator

benjamn commented Jan 21, 2021

@Jack-Works What do you think about this alternate approach? #378 (comment)

@Jack-Works
Copy link
Author

@Jack-Works What do you think about this alternate approach? #378 (comment)

@benjamn hi that's a cool trick but in some environment like moddable (a JS engine for embedded env) the Object.prototype is frozen. You cannot add things to it. Please at least leave the globalThis try.

@jridgewell
Copy link

jridgewell commented Jan 22, 2021

I'd recommend using all of globalThis, self, and global before resorting to a monkey patch on Object.prototype.

I have a sneaking suspicion that it will cause a massive deoptimization is everything but the newest browsers. The delete is going to turn the Object.prototype into map-mode, which will slow down every single object access. All of them, everywhere, and everything inherits from Object.prototype.

@zloirock
Copy link
Contributor

I'm proposing it as the most stable and tested way one more time: https://github.com/zloirock/core-js/blob/v3.8.3/packages/core-js/internals/global.js

@mikecann
Copy link

Has this PR stalled? Would love to see it go in for those of us tring to build things in CSP environments.

@Jack-Works
Copy link
Author

I'm just waiting for the maintainer to respond. There aren't so many things I need to up-to-date with.

@edwardxia
Copy link

edwardxia commented May 8, 2021

@Jack-Works This PR will fail Content Security Policy, please check the better way that @zloirock has linked in previous comment.

@Jack-Works
Copy link
Author

@Jack-Works This PR will fail Content Security Policy, please check the better way that @zloirock has linked in previous comment.

The original version also violates CSP. I'm making it better on modern browser to use globalThis directly so it don't have to violate CSP

@Jack-Works
Copy link
Author

I have rebased to master to resolve merge conflicts.

@Jack-Works Jack-Works changed the title feat: make it strict mode compatible if possible feat: make it strict mode & CSP compatible if possible May 8, 2021
@Jack-Works
Copy link
Author

@Jack-Works This PR will fail Content Security Policy, please check the better way that @zloirock has linked in previous comment.

Updated

@juergba
Copy link

juergba commented Jul 29, 2021

Is this issue still relevant? Or is it fixed in regenerator-runtime v.0.13.9?
Thank you

@zloirock
Copy link
Contributor

It's fixed only for engines with globalThis.

@poacher2k
Copy link

I keep snoozing this tab "Until next month", hoping every time it re-appears that the PR will have been merged. This really seems like a good fix for the described issue, in addition to being backwards-compatible.

On a side note, I can't remember ever actually have gotten very pissed off when debugging, but when I was debugging the strange CSP issue I had almost a year ago, and finally finding out the reason things were failing, the snarky comment that this PR thankfully removes - certainly had me swearing.

Anyhow, I'm crossing my fingers this will be fixed in January! :)

@Jack-Works
Copy link
Author

image

I tried as many names as possible. Not only globalThis now.

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

Successfully merging this pull request may close these issues.

None yet

9 participants