Fix optional arguments

[MrSaints]

The git commit description probably explains it best.

---

Update API template: use structs for params instead of arguments Previously, all parameters were passed through method arguments.

This made it difficult to exclude optional parameters such as the context ID for Runtime.Evaluate. In the described case, specifying 0 was not enough for it to be considered 'empty'. It was simply treated as a value.

This change arguably makes usage of this package slightly more verbose as a struct for every method's param will have to be initialised. Nevertheless, the effects of this is somewhat counterbalanced by the fact that we had to already do this before when specifying every argument for a method call (even for params we do not intend on using). Taken together, this may actually simplify the API's usage, and make it more idiomatic (by defining a structure for every method's properties rather than a generic map interface).

As there were conflicting method names against different types, e.g. DeleteCookie, it was not enough to just append Params to the method name to generate the struct's name. The domain is prepended for extra safety / to avoid conflicts.

Unfortunately, this change entails importing gcdapi to access the types when using the API. A solution to this may be to create 'initialisers' against a service, e.g. Page.NewDeleteCookieParams(..)

Another approach may be to use functional options, but that route may result in unnecessary bloat in the generated library. This approach is easier to comprehend.

---

The danger of this change is that we'll break all existing API usage.

Closes #6

[MrSaints]

Pending test fixes, but just thought I make this PR first to get your thoughts.

[wirepair]

Oh wow, thanks for looking into this. I agree this is a much better way of doing it. I'll need to go over this PR in a bit more detail. I would like to be able to hide the gcdapi internals but, yeah i'm not entirely sure how we can get around that using this method. I'll give it some thought. But yes, please continue with your tests. Also, if you want to get an idea of how this would impact libraries that would use this new design please see https://github.com/wirepair/autogcd as a reference (if you haven't already).

[MrSaints]

I've just pushed some tests.

Yes, I'd like to hide the internals a bit too. Functional options might be the way to go, but I'm just concerned about bloat. As for backwards compatibility, one option would be to use both 'techniques' in parallel until such a time most libraries can migrate away.

[wirepair]

OK I just checked out and ran the tests, looks great. Although it is a bit odd to pass nil to a method, but i'm not sure there's much we could do about that. I would still like to spend some time thinking about how to hide the internals/version compatibility so we can reduce how much api breakage there will be. I guess one option is to have the gcdapigen spit out two sets of methods, appending WithParams for the new style, allowing callers to choose if they want to pass the struct parameters or not.
So you could have like: network.Enable(-1, -1) or: network.EnableWithParams(&networkParams)

Thoughts?

[MrSaints]

@wirepair, yes, that's exactly what I had in mind as well to reduce breakage. The good thing about this is that methods with less arguments (or even one) can remain simple. But, I think there should still be a long-term plan, especially to avoid redundancy in the API.

[MrSaints]

@wirepair I've made the changes, and reverted changes to the tests (since they should work as we are not making backwards incompatible changes). The methods will now be proxied through the WithParams ones.

[wirepair]

This is perfect, I just need you to remove the gcdapigen binary as part of this commit and then I can merge it.

As for long term, it may make sense to conditionally determine if a method should take params (has ignorable fields) or should use arguments (has single/all required args). My only concern is that it would be a bit confusing to know which is which, I'd honestly be happy with just having the ability to choose both as an end user.

[wirepair]

Oops disregard about the gcdapigen binary, looks like i added that accidentally in one of my commits!

[MrSaints]

Yes @wirepair, I'm quite content for now since there are some methods that only takes in one argument, and it's far easier to just call those methods than to initialise params to pass into the method.

Cheers for the merge.

[wirepair]

@MrSaints Quick question, I noticed you are also submitting fixes to chromedp, any reason you are using both?

[MrSaints]

@wirepair I'm trying to find a decent Chrome Debugging Protocol client in Go.

My criteria is that: 1. it should be auto-generated so that I can get free, and easy updates even if the client is no longer actively maintained; 2. it should be simple to use (with some high-level wrappers); 3. it should be production-ready insofar as there are tests, and very few opened issues / bugs; and 4. it should be opened for contributions (slightly related to point 3, since it would suggest these issues are closed out a lot quicker).

I would write my own, but it seemed like re-inventing the wheel, and a waste of resources.

I started with chromedp since I came across it quite a while back. But, after using it, I realised the API, and high-level wrappers were slightly confusing, and deficient (not entirely though, it is still quite decent). Nevertheless, I had already begun working with it before coming to that conclusion, and I did not want to give up on it so quickly. Thus, I made a few fixes on my local copy of it. Figured it would be selfish of me if I did not push the fixes out.

I was not content however, with the end-result. And I wanted to see if there were other clients available. I came across yours after a quick Google. It pretty much meets everything in my criteria. The only thing that annoyed me was the optional params, which is now addressed.

Even though it was not part of my criteria, another requirement was that it should have helpers to easily launch an instance of Chrome to run against, and simultaneously, the ability to connect to an existing instance. gcd definitely provides me with such a facility without much complexity. I tried doing the same with chromedp, and the result was slightly messy. Also, it had issues closing / terminating when in headless mode. gcd does not.

TL;DR: I am just testing different clients, and submitting fixes where I can. I might have to settle on one soon.

[wirepair]

Interesting, thank you for the detailed write up. As for connecting to instances, that was also required of someone for my autogcd project https://github.com/wirepair/autogcd/blob/master/autogcd.go#L103. It may make sense to add this functionality directly to gcd...

Let me know if there is other functionality you think would be beneficial.