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

Check for undefined on browser globals. #462

Merged
merged 2 commits into from May 18, 2017
Merged

Conversation

@marbemac
Copy link
Contributor

@marbemac marbemac commented May 18, 2017

Not all environments include these globals. For example, web workers do not have global window objects.

This seems to be a regression from this commit: cae07b7

Not all environments include these globals. For example, web workers do not have global window objects.
@marbemac marbemac mentioned this pull request May 18, 2017
@coveralls
Copy link

@coveralls coveralls commented May 18, 2017

Coverage Status

Coverage remained the same at 63.804% when pulling 850749f on marbemac:master into 6bb07f7 on visionmedia:master.

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented May 18, 2017

👍 I mentioned this in the PR, but I think it got merged before it was read.

@marbemac
Copy link
Contributor Author

@marbemac marbemac commented May 18, 2017

Ah yeah I see it there now. Yeah this little issue just bit us in the ass, because all sorts of dependencies use debug, and we're using some of them in web workers, which means they're throwing exceptions now :/.

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented May 18, 2017

@marbemac could you remove the second check for the tests that you've updated? We don't need a truthiness test for window if we're already checking for typeof window !== 'undefined' right before that. Then I can merge + tag.

@marbemac
Copy link
Contributor Author

@marbemac marbemac commented May 18, 2017

I think part of the idea in that last commit was that the undefined check does not cover null. Without that second check, window.process will throw an exception if window is null.

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented May 18, 2017

In my experience those values will ever be null.

Doesn't hurt to be defensive I guess, but the point of cae07b7 was to clean up these checks a bit.

@marbemac
Copy link
Contributor Author

@marbemac marbemac commented May 18, 2017

I'm happy to take it out! Shall I? Or leave it?

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented May 18, 2017

@marbemac
Copy link
Contributor Author

@marbemac marbemac commented May 18, 2017

All set!

@coveralls
Copy link

@coveralls coveralls commented May 18, 2017

Coverage Status

Coverage remained the same at 63.804% when pulling 695ce4d on marbemac:master into 6bb07f7 on visionmedia:master.

@pmeijer
Copy link

@pmeijer pmeijer commented May 18, 2017

👍

@TooTallNate TooTallNate merged commit 2482e08 into visionmedia:master May 18, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 63.804%
Details
@pmeijer
Copy link

@pmeijer pmeijer commented May 18, 2017

When will this be released?

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented May 18, 2017

2.6.8 published

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.