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

Update browser support check #2560

Merged
merged 4 commits into from
Mar 24, 2016
Merged

Update browser support check #2560

merged 4 commits into from
Mar 24, 2016

Conversation

koenpunt
Copy link
Collaborator

@koenpunt koenpunt commented Mar 2, 2016

Old

  browser_is_supported: function() {
    if (/iP(od|hone)/i.test(window.navigator.userAgent)) {
      return false;
    }
    if (/Android/i.test(window.navigator.userAgent)) {
      if (/Mobile/i.test(window.navigator.userAgent)) {
        return false;
      }
    }
    if (/IEMobile/i.test(window.navigator.userAgent)) {
      return false;
    }
    if (/Windows Phone/i.test(window.navigator.userAgent)) {
      return false;
    }
    if (/BlackBerry/i.test(window.navigator.userAgent)) {
      return false;
    }
    if (/BB10/i.test(window.navigator.userAgent)) {
      return false;
    }
    if (window.navigator.appName === "Microsoft Internet Explorer") {
      return document.documentMode >= 8;
    }
    return true;
  }

New

  browser_is_supported: function() {
    if ("Microsoft Internet Explorer" === window.navigator.appName) {
      return document.documentMode >= 8;
    }
    if (/iP(od|hone)/i.test(window.navigator.userAgent) || /IEMobile/i.test(window.navigator.userAgent) || /Windows Phone/i.test(window.navigator.userAgent) || /BlackBerry/i.test(window.navigator.userAgent) || /BB10/i.test(window.navigator.userAgent) || /Android.*Mobile/i.test(window.navigator.userAgent)) {
      return false;
    }
    return true;
  }

even though if/else tends to be faster, the resulting js of this switch is more concise
, /BlackBerry/i.test(window.navigator.userAgent)
, /BB10/i.test(window.navigator.userAgent)
false
when /Android/i.test(window.navigator.userAgent)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nb] Why not /Android/i.test(window.navigator.userAgent) and /Mobile/i.test(window.navigator.userAgent); here?
I find it slightly harder to skim read what's going on like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(some sort of) consistency; this way all tests are on their own line

@tjschuck
Copy link
Member

tjschuck commented Mar 2, 2016

even though if/else tends to be faster, the resulting js of this switch is more concise

Should we care about the resulting JS?

@koenpunt
Copy link
Collaborator Author

koenpunt commented Mar 2, 2016

I'm actually not sure if if/else in JS is faster than a switch, but I know that it's the case in some languages.

@koenpunt
Copy link
Collaborator Author

koenpunt commented Mar 2, 2016

And I assumed the resulting JS to be less bytes, but it turns out that's not the case either 🤔

image

image

Because UglifyJS apparently can optimize that if to a long chain of ternary operators..

@koenpunt
Copy link
Collaborator Author

koenpunt commented Mar 2, 2016

I've reverted to an if, with less duplication

@koenpunt
Copy link
Collaborator Author

koenpunt commented Mar 2, 2016

Maybe we should put the "Microsoft Internet Explorer" test on top, because regexes are more expensive. Although this could also be seen as micro-optimization..

@stof
Copy link
Collaborator

stof commented Mar 2, 2016

@koenpunt moving it to the top is a good idea. It may even help UglifyJS optimizations, as it will see a ìf (...) {return false } return true and be able to shorten it to return !(...)

@koenpunt
Copy link
Collaborator Author

koenpunt commented Mar 2, 2016

46 characters less than the current:
image

@koenpunt koenpunt changed the title replace if/else for switch Update browser support check Mar 2, 2016
/Windows Phone/i.test(window.navigator.userAgent) or
/BlackBerry/i.test(window.navigator.userAgent) or
/BB10/i.test(window.navigator.userAgent) or
/Android.*Mobile/i.test(window.navigator.userAgent)
Copy link
Member

Choose a reason for hiding this comment

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

@koenpunt There's a subtle change here — this will no longer match the opposite order, e.g. Mobile blah blah Android, which the original did.

Not sure if anything has a user agent like that, but just pointing out that this is not 100% equivalent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, but Mobile always appears after Android according to https://developer.chrome.com/multidevice/user-agent

Copy link
Member

Choose a reason for hiding this comment

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

👍

@pfiller
Copy link
Contributor

pfiller commented Mar 24, 2016

I tend to prefer readability over micro optimizations that save us a few characters. In this case, however, it seems we get both! I find the changes slightly more readable so I'm 👍

Gonna merge it in. Thanks @koenpunt!

@pfiller pfiller merged commit a5135db into master Mar 24, 2016
@pfiller pfiller deleted the if-to-switch branch March 24, 2016 22:25
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.

5 participants