Skip to content

handle undefined document, window variables in BrowserPlatform #1139

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

Closed

Conversation

Bunkerbewohner
Copy link

Simple fix for the problem described in #1138

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@Bunkerbewohner
Copy link
Author

I signed the CLA!

@googlebot
Copy link

CLAs look good, thanks!

@wilhuff
Copy link
Contributor

wilhuff commented Aug 18, 2018

In principle this seems like a good change, thanks for this!

@schmidt-sebastian Could you double-check that this is right?

@schmidt-sebastian
Copy link
Contributor

schmidt-sebastian commented Aug 18, 2018

Thanks for sending this over. I would like to go one step further and verify that "window" and "document" are known types before accessing them. I have seen some platforms (such as Node) that error even when checks like "window || null" are performed.

See follow-up PR #1140.

@Bunkerbewohner
Copy link
Author

Ooops seems like I accidently pushed the wrong code and didn't even notice it. My bad. You are of course right that just ORing with null is not sufficient. Thanks for the follow up PR!

I updated my PR just for the sake of correctness.

@schmidt-sebastian
Copy link
Contributor

Thanks for updating this. I have merged my follow-up PR, which uses the same logic you have added in this PR. I will close this PR as #1140 is merged. Thanks!

@firebase firebase locked and limited conversation to collaborators Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants