-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
cli(extra-headers): Enable sending additional HTTP Headers via the CLI #3732
Conversation
# Conflicts: # lighthouse-cli/bin.ts # lighthouse-core/gather/gather-runner.js # lighthouse-extension/app/manifest.json # package.json
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
lighthouse-cli/cli-flags.ts
Outdated
@@ -14,7 +14,8 @@ import {GetValidOutputOptions, OutputMode} from './printer'; | |||
export interface Flags { | |||
port: number, chromeFlags: string, output: any, outputPath: string, saveArtifacts: boolean, | |||
saveAssets: boolean, view: boolean, maxWaitForLoad: number, logLevel: string, | |||
hostname: string, blockedUrlPatterns: string[], enableErrorReporting: boolean | |||
hostname: string, blockedUrlPatterns: string[], enableErrorReporting: boolean, | |||
extraHeaders: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Hmm this should probably be of type string
I signed it! |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Merge away. Glad this commit is going to see the light of day! Good stuff @rupesh1 |
@paulirish This PR looks useful for both CLI and programmatic use. If the conflicts are resolved, is there a reason it can't be merged? I know there were other issues where we were discussing using Chrome's setCookie. |
This feature would be handy for us: we have conditionals in our code the can be activated/deactivated using HTTP Headers. Specifically cookies. Our current workflow is:
We could script this behaviour using the Chrome Debug Protocol, but ... wouldn't it be sweet if you could just pass an option into 💡 🏠 ? ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR @rupesh1!!
sorry for the long delay but it seems to be quite popular these days! 🎉 we like the idea and just need some 🚲 🏠 on the CLI usage a bit
lighthouse-core/gather/driver.js
Outdated
setExtraHTTPHeaders(jsonHeaders) { | ||
let headers; | ||
try { | ||
headers = JSON.parse(jsonHeaders); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about handling this in the CLI instead?
this way we could fail early and also do something fancy like accept files :)
let extraHeaders
if (argv.extraHeaders) {
if (argv.extraHeaders.substr(0, 1) === '{') {
extraHeaders = JSON.parse(argv.extraHeaders)
} else {
extraHeaders = JSON.parse(fs.readFileSync(argv.extraHeaders))
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rupesh1 holler if you can make this change. if you've moved on, we can pick it up;
lighthouse-core/gather/driver.js
Outdated
log.warn('Driver', 'Invalid header JSON'); | ||
headers = {}; | ||
} | ||
return this.sendCommand('Network.setExtraHTTPHeaders', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably skip this entirely if headers is invalid/empty instead
@patrickhulce @paulirish I've had a go at implementing your suggestions (everything passes and runs locally for me node --version 8.9.1, yarn --version 1.3.2), but now bumped in to some typings issue with yargs when the CI runs
I can't reproduce the error locally so any help is appreciated. |
Thanks for updating @rupesh1!! It looks like you might have a newer version of yargs installed locally in how about we move the parsing logic over to bin.js for now instead? |
@patrickhulce thanks for the tip, CI is looking much better now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks awesome! great job @rupesh1! 🎉 💯
everything a-ok by me, @paulirish over to you for any opinions on the CLI usage
I'm not 100% on using a file rather than a string, but neither seems like the clear winner. Let's do this. |
thanks @rupesh1 ! |
Awesome! Really appreciate everyone's help with this PR 👍 |
This is great. Is there any example of the format required for the json file? |
@JoelTW it should just be key/value header name/value {
"Cookie": "token=12345acdfe",
"x-secret-header": "it's a secret"
} |
Is there an example we can use these extra headers through programmatical Lighthouse? |
I can across #2746 which mentioned a SO question, that had a link to commit by @fdn - after taking a look it seemed abandoned but functional. So based off their initial commit I've:
I've tested the CLI locally and it's working as expected for me.
P.s. I had to run
yarn install-all
a few times for it to run cleanly hence adding yarn-error.log to the gitignore