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.