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

Add userAgent option #303

Closed
tamorim opened this issue May 2, 2018 · 3 comments
Closed

Add userAgent option #303

tamorim opened this issue May 2, 2018 · 3 comments
Assignees

Comments

@tamorim
Copy link
Contributor

tamorim commented May 2, 2018

Hello! First of all, thanks to all the contributors to this project. 💯

I'm stepping in some edge case here, I guess.
The app I'm working on right now is adaptive without different urls for mobile, so I pass the userAgent option to penthouse where he applies it just fine.
Since I only provide the src (no css files or strings) as a remote url, critical goes and fetches the html file and extracts the css files from the html.
The problem is that when critical goes to fetch the html file, since it does't use any custom user agent, it retrieves the "default" page which doesn't contain the desired css file for that case, generating "wrong" critical css output.

What do you guys think? Is it possible to add a option like this?
I thought that it would be useful and serve as an abstraction to the penthouse.userAgent as well.
I'm willing to open a PR for it if that kind of option is desired.

@raxbg
Copy link

raxbg commented Jun 14, 2018

I agree completely. In fact I have seen many sites that behave like that. I made a small change to the file-helper.js which allows you to specify the user agent through an environment USER_AGENT variable. Please see the referenced pull request.

I hope that we will get official support for this in the next release.

@bezoerb bezoerb self-assigned this Jun 14, 2018
@tamorim
Copy link
Contributor Author

tamorim commented Jun 15, 2018

@raxbg I hope that you don't mind that I've opened a PR myself too.
I'm already rolling with a critical fork with the userAgent option for some weeks at my job.
Seeing your comment gave me some strength to rebase and add a test for it haha, I hope that you like the solution too!
I'm open to any feedback

@raxbg
Copy link

raxbg commented Jun 15, 2018

@tamorim Thanks for sharing this. It's exactly what was needed 👍

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

No branches or pull requests

3 participants