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

implementation of a new result type: SearchBotDeviceInfo #127

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

DamonOehlman
Copy link
Owner

A potential implementation that addresses the request in #126

From an implementation perspective I've implemented a result class - SearchBotDeviceInfo which adds the bot string so that the result of the detection of the UA:

Mozilla/5.0 (Linux; Android 6.0.1; Nexus 5X Build/MMB29P) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.96 Mobile Safari/537.36 (compatible; Googlebot/2.1; +http://www.google.com/bot.html)

is:

{
  name: 'chrome',
  version: '41.0.2272',
  os: 'Android OS',
  bot: 'Googlebot',
}

In terms of being able to determine that we have captured one of these new types of results, either the bot attribute can be tested, e.g. typeof result.bot == 'string' or using the instanceof check given the result is a class. Using a truthy check on the bot attribute will also pickup those instances of standard search bots that aren't simulating devices also.

This is starting to increase the complexity of the result type, and I'm not really one for doing instanceof checks so I think it might be time to add a kind discriminator also.

I'd be keen to bump major on this too if we decide this is a good way to go.

@DamonOehlman DamonOehlman requested a review from 5punk December 15, 2019 22:50
@5punk
Copy link
Collaborator

5punk commented Dec 16, 2019

LGTM 👍

@DamonOehlman DamonOehlman merged commit 33a4e24 into master Dec 17, 2019
@DamonOehlman DamonOehlman deleted the damon-searchbot-device-info branch December 17, 2019 01:56
@DamonOehlman
Copy link
Owner Author

Merged this down but will add the discriminator before releasing a new major version.

This was referenced Dec 17, 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.

2 participants