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

Check for undefined on browser globals. #462

merged 2 commits into from May 18, 2017

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 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
Contributor

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

@marbemac
Copy link
Contributor Author

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
Contributor

@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

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
Contributor

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

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

@TooTallNate
Copy link
Contributor

TooTallNate commented May 18, 2017 via email

@marbemac
Copy link
Contributor Author

All set!

@coveralls
Copy link

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 commented May 18, 2017

👍

@TooTallNate TooTallNate merged commit 2482e08 into debug-js:master May 18, 2017
@pmeijer
Copy link

pmeijer commented May 18, 2017

When will this be released?

@TooTallNate
Copy link
Contributor

2.6.8 published

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

Successfully merging this pull request may close these issues.

None yet

4 participants