switch to using hato

[loomis]

Connected to oliyh/re-graph#58.

Points to discuss/fix before merging:

• [x] ws-parameters: ok, move specific parameters to top-level, move other ws parameters into ws-parameters, ...
• [ ] possibly making HTTP library pluggable
• [x] mocking hato to bring back functional test
• [x] use non-snapshot version of hato with WebSocket support (see gnarroway/hato#7)

[oliyh]

Something I'm considering, as the number of settings have grown, is to refactor the options map into something more like this:

{:ws {:url "..."
      :sub-protocol "..."
      :reconnect-timeout 6000
      :init-payload {...}
      :resume-subscriptions? true}
 :http {:url "..."
        :request-parameters {...}}}

Would that help implementing this?

[oliyh]

Looks good so far, let me know when it's ready to review. Thank you again for your contributions!

[loomis]

Would that help implementing this?

From a usability standpoint, I think that reorganizing the parameters like you've suggested is good. For implementing the switch to hato, the change doesn't have much impact. If you'd like it done as part of this PR, I can do it or I can help with it as a second change after this PR is finished.

[loomis]

Concerning making the clojure HTTP client pluggable, I don't see much value in this in the end. With the old implementation, I've run into the cookie issue mentioned in the referenced ticket and also conflicts with the jetty versions used by gniazdo and lacinia-pedestal. With the hato implementation, there are fewer dependencies and it is possible to easily configure a shared cookie store. Provided that there are no underlying bugs with the JDK 11 http/websocket clients, I don't see any downsides to the switch. The end-user of re-graph probably doesn't really care about the implementation.

[loomis]

@oliyh Go ahead and take a look at the changes. The client works on my project, but would be happy to have more testing in other environments.

[loomis]

@oliyh The lasts few commits implement the change in the options schema. Again, this works on my project, but more testing would be welcome.

[oliyh]

Thank you for this, it looks good.

I think what I'd like to do it bump the version number when releasing this change as it introduces two breaks:

• JDK version
• API for ws and http parameters

I would also like to explore the pluggable option before releasing this as noticed from the State Of Clojure 2020 results that a very large number of people are still using JDK8. I'm assuming however that most users only use the cljs implementation and I think that there should be no issues compiling this in cljs even if the compiling JDK is 8, and it would not evaluate the clj branch in that case.

[loomis]

@oliyh If you want the http/ws implementation to be pluggable, it would be rather straightforward to create a protocol to hide the underlying differences in the implementations. There would need to be three methods for the WebSocket (connect, send, and close) and one for the HTTP request (post). The clj-http/gniazdo and hato implementations are similar enough that the protocol implementations would be trivial to write. The user could choose the implementation either explicitly at initialization or implicitly by including a namespace for the desired implementation.

Is this along the lines of what you were thinking of?

For the dependency management, I don't an easy solution though. I don't think it would be a good idea to include the dependencies of all possible implementations (especially as gniazdo has caused problems for me with jetty). One possibility would be to package the implementations separately. This makes the build/publish process more complicated. But the users could select the implementation easily by just adding an additional explicit dependency in their project.clj or deps.clj files.

[oliyh]

Hi @loomis

Sorry for the delayed response. I think the ideal approach would be able to create two new packages, one called re-graph.java-8 containing the clj-http/gniazdo implementations and re-graph.hato with your implementation. re-graph will continue to include all the main code and depend on re-graph.hato by default. I have done something like this for my martian library.

I would like to do a new release of the current code before doing this split and then I can work on integrating this. I'm concious of possibly blocking you before this work can be merged and released?

Thanks

[oliyh]

I have now done the 1.12 release

[loomis]

@oliyh I'm not blocked by any delays in merging this into master. I'm using a SHA dependency and taking the code directly from GitHub. If there are specific tasks or changes that you'd like done, let me know. I'll have a bit of time in the next few days to work on this.

[oliyh]

Hi @loomis

I have finished #63 which is an integration test suite with a real graphql server, which I wanted to do before changing the implementation. I should now be ready to start work on looking at how to package this.

[oliyh]

Hi @loomis

I merged your changes to the hato branch and made the implementations pluggable. I made a small change to your README changes, could you check that it's now correct? https://github.com/oliyh/re-graph/pull/66/files#diff-04c6e90faac2675aa89e2176d2eec7d8R173-R174

And let me know if everything else on that branch looks good to you. If so I will merge back to master and do a release (0.2.0 probably given the API changes).

Thanks for your work and patience!

[loomis]

@oliyh I’ll try to test the changes this evening.

[oliyh]

This branch was continued in #66 which is now merged