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 the regexp for generic browsers #310

Merged
merged 10 commits into from
Apr 24, 2019
Merged

Update the regexp for generic browsers #310

merged 10 commits into from
Apr 24, 2019

Conversation

ricardorauber
Copy link

@ricardorauber ricardorauber commented Apr 5, 2019

Hi @lancedikson,

First of all, thanks for the awesome module!

Second, I was trying to set a custom user-agent for my iOS app iOSApp/3.0 (iPhone; CPU iPhone OS 12_1_4 like Mac OS X) and right now the result is browser.name = iOSApp and browser.version = 3.0 (iPad; CPU iPhone OS 12_1_4 like Mac OS.

I noticed that this is happening because of the regexp on parser-browser.js:594-595, so I came up with a slightly different regexp to get the correct data. With this, I was able to get browser.name = iOSApp and browser.version = 3.0.

Browser name: iOSApp
Browser version: 3.0
OS name: iOS
OS version: 12.1.4
Platform type: mobile
Platform vendor: Apple
Engine name: undefined

I hope this could help many other people as well :)

@ricardorauber ricardorauber changed the title Update the regex for custom browsers Update the regexp for custom browsers Apr 5, 2019
@coveralls
Copy link

coveralls commented Apr 5, 2019

Pull Request Test Coverage Report for Build 551

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 88.785%

Files with Coverage Reduction New Missed Lines %
src/parser.js 1 89.24%
src/parser-browsers.js 1 85.49%
src/utils.js 4 89.04%
Totals Coverage Status
Change from base Build 488: 0.2%
Covered Lines: 496
Relevant Lines: 512

💛 - Coveralls

@ricardorauber ricardorauber changed the title Update the regexp for custom browsers Update the regexp for generic browsers Apr 5, 2019
Copy link
Collaborator

@lancedikson lancedikson left a comment

Choose a reason for hiding this comment

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

Hi, @ricardorauber, thanks for the PR. Seems pretty reasonable to have this fix, but I'd ask you to leave a comment in the codes about the solution.

@@ -590,9 +590,10 @@ const browsersList = [
{
test: [/.*/i],
describe(ua) {
const regexp = ua.search('\\(') === -1 ? /^(.*)\/(.*) / : /^(.*)\/(.*)[ \t]\((.*)/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you, please, leave a comment in the code with some details on why exactly we use this ua.search('\\(') === -1 here? :)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sure, I can definitely do that! The idea is to use the search on the string to check if it has the "(" char, in case there is a device information in the User Agent, otherwise use the old separator. Is it ok to put it like that or should I be more technical?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest creating a variable like const hasDeviceSpec = ua.search('\\(') !== -1; and put a comment for it, that here we try to make sure that there are explicit details about the device in order to decide what regexp exactly we want to apply (as there is a specific decision based on that conclusion). It would make the code more understandable in the future :) Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

hey @lancedikson, sorry for the delay, but I have made the changes, what do you think?

Copy link
Collaborator

@lancedikson lancedikson left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks! Will merge it and release a new version soon!

@lancedikson lancedikson merged commit e1a37ef into bowser-js:master Apr 24, 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