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

Pass cdp options to CDP.new() #103

Merged
merged 2 commits into from
Aug 2, 2017
Merged

Conversation

liady
Copy link
Contributor

@liady liady commented Jul 30, 2017

pass the cdp options from the instantiation to CDP.new()

pass the cdp options from the instantiation to CDP.new()
@timsuchanek
Copy link
Contributor

Thanks for creating this PR @liady! Passing this in makes sense, besides one option which is closeTab https://github.com/graphcool/chromeless/blob/master/src/types.ts#L22
We first should move closeTab to a different place in the options, then this PR can be merged.
I create a seperate issue for that task

@liady
Copy link
Contributor Author

liady commented Aug 1, 2017

@timsuchanek do you prefer moving closeTab one level higher, to the root options object? Won't it break users' code that is currently using this API?

@joelgriffith
Copy link
Contributor

Just merged another PR, might have some conflicts, let me know if I can help

@timsuchanek
Copy link
Contributor

@liady that's a fair point. We discussed it with the maintainers and concluded, that closeTab should still live in the CDP option.

We should, however, filter that option out before passing into CDP.New

With something along the lines

// filter out closeTab
const {host, port, secure} = this.options.cdp
CDP.New({host, port, secure})

Could you incorporate that? Thanks!

@liady
Copy link
Contributor Author

liady commented Aug 2, 2017

@timsuchanek thanks, changed.
@joelgriffith can you please review my conflict resolving to see if it complies with your PR?

@timsuchanek timsuchanek merged commit f9a4b6e into schickling:master Aug 2, 2017
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.

4 participants