Bonito connection

[frankier]

This incorporates #211

Currently if you use WGLMakie/Bonito together with an interactive plot, Bonito will start a websocket server locally. It's nicer to have the endpoint managed by Oxygen so it's on the same port, can easily be put behind auth and so on.

Here's an example of how to use it

function __init__()
    @info "Setting up Bonito to use Oxygen websockets"
    @websocket "/bonito/{session_id}" Oxygen.bonito_websocket_handler
end

@get "/" function plot(req)
    WGLMakie.activate!()
    force_asset_server!(NoServer())
    force_connection!(Oxygen.OxygenWebSocketConnection(getexternalurl() * "/bonito/"))
    app = App() do session::Session
        # Plotting commands
    end
    app_html = sprint(io-> show(io, MIME"text/html"(), app))
end

This needs to be worked on a bit more. Better API, docs, tests, and also timeout management/session evication.

First I will open a ticket with Bonito to see which parts of the API are public in case it is possible to share more code.

Comments are welcome already though.

[frankier]

Another wishlist here is hooking Bonito into the same hot-reload logic as Oxygen.

[frankier]

Updated based on Bonito #244

[ndortega]

Hi @frankier,

Thanks for pushing for this feature, and I like the idea of introducing this as an extension. I've taken a peek at your other PR you've opened up on the Bonito.jl repo.

The only question/feedback I had was around these values used in the extention:

const open_connections = Dict{String, Session{OxygenWebSocketConnection}}()
const open_connections_lock = ReentrantLock()

How would this impact package developers who want to build a package on top of Oxygen? In general, we try to keep all stateful values inside the current context object to prevent values/configuration from clashing.

[frankier]

Good call! I guess the Context struct would need to have a extensions dictionary of type Dict(Symbol: Any} for extensions to put data into. This is the simplest solution, but if you have a more type-stable/JET-friendly idea, I'm open to suggestions.

When constructing a OxygenWebSocketConnection a user would then pass in the current context. I think however that probably a macro that does everything would be added in this PR (as well documenting the "manual API" where endpoints can be registered manually for those who want to add some custom behavior).

This all depends on a few bits and pieces though. Ideally we could first get #211 merged, and then if you agree I could make a PR adding the dictionary to the context object and add some docs for that.

[ndortega]

@frankier,

No worries about the type stability of the "extensions" dictionary. The feels like a good first step, once we get things working then we can try to address this later on with better dispatching logic.

At the very least the extension that is accessing this dictionary can add some getters and setters with the appropriate input/output types.

[frankier]

This seems to work okay now and seems feature complete. I've tested it manually, but adding some kind of smoke test would be nice.

What would you advise? I could of course easily call a couple of methods to test nothing throws an exception, but to actually test the connection properly a browser is needed. Bonito includes even a small framework for this: https://github.com/SimonDanisch/Bonito.jl/blob/master/test/ElectronTests.jl

This would be adding quite a bit of complexity to the tests, and again, it would at least be nice to reuse stuff from Bonito.jl, but this probably means asking @SimonDanisch to put ElectronTests.jl into a package, which he might not be ready to do. Otherwise I would probably vendor it for now.

What do you think? Might a low quality smoke test without a client be enough to make this mergable short term?

By the way, the other thing is that the API isn't absolutely amazing right now, including the requirement to import Bonito before Oxygen. This is why I put it as experimental in the docs, since some stuff will have to change when Requires.jl is dropped (it is unmaintained following the introduction of package extensions making it defacto deprecated).

[frankier]

Actually this isn't ready since it depends upon SimonDanisch/Bonito.jl#253 so first that needs to be merged, a Bonito release made and the Bonito compat updated or this functionality put behind a version check. (Let me know which you prefer.)

[Azzaare]

Sorry to bump this PR, but how is it going?

[frankier]

I think I was stalled for a couple of reasons. One was dealing with Julia 1.6, but looks like that's dropped now, which is good, but means a bit of reworking into an extension. The other is that I encountered various jankiness when trying to make examples for the docs generally to Bonito's "lifecycle" including SimonDanisch/Bonito.jl#260 and wasn't really sure whether I would be able to fix anything on the Bonito side. I think on balance it might be possible to work around these in the examples for now so I might give it another go soonish -- thanks for reminding me.

[ndortega]

Hi @frankier,

Thanks for taking the time to rework it into an extension, right now the main thing missing is the unit tests for this. Like you mentioned programmatically testing a dynamic plot is fairly complicated so I'd only ask for api level tests to make sure you can spin up a server with this integration and get some 200's out of it.

[ndortega]

Hi @frankier,

It appears their maintenacne window is over and the test are going through again. With that said we just need to get the test coverage to at least >= 98% to match the project. I hope this doesn't come off as excessive, but I personally don't push code unless it matches this level of coverage.

This is especially useful for code I personally didnt' write, as it let's me "fearlessly" refactor code.

From the current report it just looks like there's a few more cases to cover before we can merge and release it.

• Testing the allow_soft_close() case
• Have the cleanup_bonito_context() function have connections to clean up
• Test with external_url set to nothing
• Test missing :bonito_connection key in ext_context

[frankier]

I see. I understood from your previous comment that smoke tests would be enough. In this case I don't think just chasing coverage with trivial unit tests will do the trick, since the most likely possibility for failures to be introduced is that subtle changes in Bonito's behaviour could cause things to break in the future, therefore what would be more useful would be integration tests. I've opened an issue over at Bonito SimonDanisch/Bonito.jl#293 to ask about getting the Electron based webdriver-like as a package, after which it should be easy to adapt some tests from Bonito's own testsuite which should exercise both code paths in Oxygen, Bonito and their interaction.

[frankier]

I tried writing these tests but I'm not entirely sure how it should go. (Although this could be because I'm currently recovering from BPPV so not at full capacity.) I think the reason is that this is a bit experimental and mechanisms such as soft close are sort of a hack and not necessarily the best possible implementation. I'm not always sure of what the expected results are. In a nutshell, I've essentially just hacked these two libraries so that they work together and more believe this is a reasonable starting point which may need further refinement, particularly with regards to session and resources management for use in production. If you're not comfortable with this (and it does make some sense that you wouldn't be) perhaps I should just add the minimum from the Oxygen side to support this as an external extension and put this in a package instead?

[ndortega]

Hi @frankier,

Sorry to hear that, there's no rush from my end to get this merged asap. You may not need to write tests to cover every line but I generally try to cover ever major case. And I don't do this just to chase coverage for its own sake, rather this comes from all the times I've gotten burned from realeasing / depending on code without unit tests. On top of general "trust" it builds in the codebase, this also helps direct how I design my code, If I find that a new feature is really hard to test then that's a good indication that I might've made some code either too complicated or has too many side effects / dependencies.

As someone who's consuming your code, having tests shows me how you anticipate the code to be used. This is super helpful for me, because I can see the code function as you intented and use that info to create new tests to handle different cases. I also plan on working on this project over the long term and it's not possible to keep all the context around every single module in my working memory at all times, so having all my assertions written down has been beyond helpful when working on older code.

Now to make things more concrete, we can work on creating a test case to test the softclose functionality which should hit both that fuction and the cleanup function. I do have some websocket tests that we can reference and use those examples to create some clients, connect to your bonito server, request some data and then perform a softclose. This won't hit every line, but at least it will cover the main flow.