Skip to content
This repository has been archived by the owner on Nov 24, 2018. It is now read-only.

Add "setUserAgent" method to example #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daannijkamp
Copy link

Google checks on user agent, otherwise it will show another version of Google

Google checks on user agent, otherwise it will show another version of Google
@elisherer
Copy link
Contributor

elisherer commented Aug 7, 2017

You can see in my PR 137 that I changed the page to a static page served as data URI.
Relying on Google for our unit/integration tests is not a good thing.
(EDIT: removed reference to PR because of misreading the PR)

Copy link
Collaborator

@adieuadieu adieuadieu left a comment

Choose a reason for hiding this comment

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

Hi @daannijkamp thanks for raising this PR. I'm not sure I agree with it. While it's useful to be able to set the user-agent for testing one's own websites (which is why we have the setUserAgent() method), I don't think we should actively encourage faking user-agent's in our examples. We want Chromeless to be a force-for-good and therefore not, by default, hide our identity as an automated user.

@elisherer the scripts in the examples/ folder are not intended to be unit tests. They're standalone examples of various use-cases.

@daannijkamp
Copy link
Author

@adieuadieu It was not my intention to fake a User-Agent. If Google doesn't recognise the User-Agent, (at all), it will show a version of Google that is different than a normal user will get.
(Ajax vs Non Ajax, different layout, different classes)

This makes the example somewhat confusing.

Maybe you can change the default User-Agent to something like
Mozilla/5.0 (compatible; Chromeless/2.1; +https://github.com/graphcool/chromeless
(https://support.google.com/webmasters/answer/1061943?hl=nl)

@elisherer
Copy link
Contributor

@adieuadieu , you are right, I misread the PR.

@joelgriffith
Copy link
Contributor

I'm not sure that gating off the user-agent is worth doing as, at some point, we'll allow access to the underlying protocol itself, thus allowing users to do so. I agree that we should be championing good behavior, just not sure to what ends we're willing to take that

@adieuadieu
Copy link
Collaborator

@joelgriffith we already support setting user-agent with the setUserAgent() method. My concern about whether we should encourage faking the user-agent when making requests to Google in one of our main example scripts. But maybe it's not really that big of a deal. I suppose setting the user-agent to something like Mozilla/5.0 (compatible; Chromeless/2.1; +https://github.com/graphcool/chromeless as suggested by @daannijkamp would let us make a statement about good behavior within the example itself. @daannijkamp — does that user-agent make Google behave normally?

@adieuadieu adieuadieu added docs and removed feature labels Aug 12, 2017
@daannijkamp
Copy link
Author

@adieuadieu
Unfortunately not, but it does make it better. Google checks if the User Agent is a valid User Agent of one of the popular browsers.

My advice is:

  • Don't use Chromeless 1.2.0 as the default User Agent. At least add the compatibility flag token
  • An alternative is to add an API to use the User Agent of Chrome (used in headless mode)(useDefaultChromeUserAgent(true))

Chromeless 1.2.0

Mozilla/5.0 (compatible; Chromeless/2.1; +https://github.com/graphcool/chromeless

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

@adieuadieu
Copy link
Collaborator

@daannijkamp changing the default user agent to Mozilla/5.0 (compatible; Chromeless/1.2; +https://github.com/graphcool/chromeless is a good idea. To your second point, rather than adding an API, we could add a Chromeless constructor option which disables Chromeless from overriding the User Agent.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants