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 chrome host as parameter #2727

Closed
salvocanna opened this issue Jul 23, 2017 · 3 comments
Closed

Add chrome host as parameter #2727

salvocanna opened this issue Jul 23, 2017 · 3 comments

Comments

@salvocanna
Copy link

I'm looking to use lighthouse (which is really well done) programmatically integrating it inside another node app and I'm having some issues as the node app and headless-chrome are running inside two different containers.

This assumes chrome is always located on localhost and allows you to change only the port.

Would it make sense to call the constructor with a config object like { host: DEFAULT_HOST, port: DEFAULT_PORT } or I'm just doing something weird and there's another way to specify the host?

@patrickhulce
Copy link
Collaborator

Not doing anything wrong, I believe you're just the first request we've had for this :)

Makes sense to have as an option though it could be plumbed through as port is now. Any chance you'd want to take a swing at a PR?

FrederickGeek8 added a commit to FrederickGeek8/lighthouse that referenced this issue Jul 24, 2017
Decided to make hostname second parameter to avoid breaking existing
implementations
@FrederickGeek8
Copy link
Contributor

I believe the pull request I just created has fulfilled your request. You might want to check it out and make sure that everything is as you want it.

Additionally, you may want to note that the hostname is the second parameter of the constructor function. I made this choice as to not disrupt and current implementations of CriConnection.

salvocanna added a commit to salvocanna/lighthouse that referenced this issue Jul 29, 2017
Convert the parameter conf to object to make it more extensible
@salvocanna
Copy link
Author

Sorry for such a late answer.
From my point of view the host should take precedence over the port, so I just moved the config in the constructor to an object with defaults.
I've also made a pull request (#2793) - whichever makes more sense

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

4 participants