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

Fix: Don't call methods that aren't available in the node environment #27

Merged
merged 2 commits into from
May 13, 2019

Conversation

jkeen
Copy link
Contributor

@jkeen jkeen commented Apr 22, 2019

I'm using tui-calendar in an ember app, and when loading it on the server it errors out when calling window.navigator.appName, since window.navigator is null in the node environment.

This change checks for window and window.navigator, and if those don't exist, it assumes it's the node environment and marks the browser object as such.

@jkeen jkeen changed the title Don't call methods that aren't available in the node environment Fix: Don't call methods that aren't available in the node environment Apr 22, 2019
@jung-han
Copy link
Member

Can one of the admins verify this patch?

@jkeen
Copy link
Contributor Author

jkeen commented Apr 26, 2019

@dongsik-yoo?

Copy link

@dongsik-yoo dongsik-yoo left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! Please check my comments.

var appName = nav.appName.replace(/\s/g, '_');
var userAgent = nav.userAgent;
if (window && window.navigator) {
var nav = window.navigator;

Choose a reason for hiding this comment

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

Can you please run eslint? Because we're using indent 4 at tui-code-snippet.

}
}
else {
browser.node = true

Choose a reason for hiding this comment

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

I think this is not enough to detect a node. There is process object to detect it. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s fair, we don’t have to assume it’s node and it might be out of scope for what this utility is supposed to do. I can skip that and only check if window and navigator exists, and make no assumptions if it doesn’t.

@dongsik-yoo
Copy link

@jung-han Please verify this PR. I leave some comments.

@jkeen
Copy link
Contributor Author

jkeen commented May 10, 2019

@jung-han I think this is ready. Once this gets merged and tui-calendar's dependencies get updated it's going to resolve an issue I'm having in a production app, so thanks!

@jung-han
Copy link
Member

@jkeen Sorry for late😢. Thanks for PR!

@jung-han jung-han merged commit 822a9da into nhn:master May 13, 2019
@jkeen jkeen mentioned this pull request Jul 20, 2019
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

3 participants