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 if window is available before using it #104

Merged
merged 4 commits into from May 11, 2017
Merged

Check if window is available before using it #104

merged 4 commits into from May 11, 2017

Conversation

thomasconner
Copy link
Contributor

Some JavaScript platforms break because of the use of window. Below are some platforms I can think of.

https://www.nativescript.org/
http://www.appcelerator.com/mobile-app-development-products/
https://facebook.github.io/react-native/ (Possibly)

Some JavaScript platforms break because of the use of window. Below are some platforms I can think of.

https://www.nativescript.org/
http://www.appcelerator.com/mobile-app-development-products/
https://facebook.github.io/react-native/ (Possibly)
lib/loglevel.js Outdated
window.document.cookie =
encodeURIComponent(storageKey) + "=" + levelName + ";";
} catch (ignore) {}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Sure, this is reasonable. I'd rather flip it and add a guard clause though, rather than indenting the whole function. Can you convert both of these into something like the below instead?

if (typeof window === undefinedType) return;

[...rest of the function...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@pimterry
Copy link
Owner

Sorry, JSHint actually doesn't like that example, so the build broke. I disagree though, so I just went and added a quick patch to relax that, and then there was an actual bug, so I fixed that while I was there too. Anyway, all looks good to me now, merging! Thanks for contributing 😃

@pimterry pimterry merged commit d3705dc into pimterry:master May 11, 2017
@pimterry
Copy link
Owner

@thomasconner Hmm, wait actually. Looking closer at this, I'm not sure why it's required. The following lines that access window are all wrapped with a try/catch, so everything should still work fine without window present anyway.

Are you fixing this because this causes an actual issue you can reproduce somewhere?

@thomasconner
Copy link
Contributor Author

Yes. When I was using loglevel in a NativeScript app it would crash because it was trying to access window.

I will try and put a project that reproduces the issue.

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.

None yet

2 participants