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

disabling document.all inside a sandbox #79

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Mar 19, 2020

As a follow up from PR #77, and after reviewing the current usage of this feature:

https://www.chromestatus.com/metrics/feature/timeline/popularity/83

image

I have reviewed some of the websites that are using this feature, all of them are just simply relying on the undefined nature of the value to detect something. It seems that the detection of the value being undefined is more important that the feature itself, and for that reason, we should favor the detection. In this PR, we are making the value undefined inside the sandbox, which means that you will never be able to access the collection itself, you have others means for that.

@caridy caridy requested a review from jdalton March 19, 2020 21:05
@caridy
Copy link
Contributor Author

caridy commented Mar 19, 2020

@jdalton initially we were saying that a user-land distortion could be the solution here, but it seems that the high usage of this (+20% and growing) is really what matters, so I'm changing my position, and making this a feature of this library.

cc @mmis1000

Copy link
Contributor

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

Let's do it!

@caridy caridy merged commit 309a1c1 into master Mar 31, 2020
@caridy caridy deleted the caridy/disabling-document.all branch March 31, 2020 02:47
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