JuliaPlots / PlotlyJS.jl

Julia library for plotting with plotly.js
Other
427 stars 78 forks source link

Revisit removal of Blink as a dependency #272

Open EricForgy opened 5 years ago

EricForgy commented 5 years ago

Hi @sglyon πŸ‘‹

I raised this issue once here

https://github.com/sglyon/PlotlyJS.jl/issues/184

but would like to raise it again.

Blink.jl is kind of a heavy dependency with a nontrivial build process. I don't think it is really necessary for what I think you want with PlotlyJS.jl.

Would you be open to using Requires.jl and have some code that only gets loaded if someone has independently loaded Blink.jl?

This way, you would not lose any Blink functionality, but Blink would not be a dependency.

When I raised this issue before, it was for my package, Pages.jl, and we were able to close the issue because it turns out I could just use PlotlyBase.jl for Pages.jl instead.

Now, I have Figures.jl. Figures.jl provides a display that will display PlotlyJS charts in a draggable "Figure" in a browser window and depends on PlotlyJS.jl proper.

My idea is to:

Reason: I don't want people to have to install Blink.jl to use my package that has a dependency on PlotlyJS.jl.

What do you think?

If you are open to this idea, I can submit a PR πŸ™

Edit: By the way, if you are open to this, I may do something similar with WebIO.jl since I'd like to use PlotlyJS.jl without acquiring a dependency on WebIO.jl as well. In that case, I might even add Figures.jl as a backend to PlotlyJS.jl with the same philosophy, i.e. using Requires.jl so PlotlyJS.jl doesn't have a dependency on Figures.jl but can take advantage of it.

EricForgy commented 5 years ago

Oh yeah. I forgot about this too :)

https://github.com/sglyon/PlotlyJS.jl/pull/41#issuecomment-370427218

EricForgy commented 5 years ago

Hi @sglyon πŸ‘‹

Quick update...

My post above was conceptual. Since then, I've actually dug into the code.

I have a local branch already working with the Blink.jl dependency removed and handled using Requires.jl so no functionality is lost. It was actually pretty simple.

Removing the dependency on WebIO is possible, but not a priority for me at this moment.

Going forward, if you are ok with it, if people want to use Blink.jl with PlotlyJS.jl, they will need to do:

using Blink, PlotlyJS

and this will work as usual. Blink.jl has dependencies on both WebIO.jl and JSExpr.jl so this will bring in those as well.

As mentioned, I've already got a working local copy of PlotlyJS.jl that works with Figures.jl. To use PlotlyJS.jl with Figures.jl, you will do this:

using PlotlyJS, Figures

This will not have a dependency on Blink.jl (which is the whole point of this issue). It is working locally, but with a deved version of WebSockets.jl so I'll hold off submitting a PR here until a version of WebSockets.jl gets merged and tagged that supports HTTP.jl v0.8.0.

Just FYI.

EricForgy commented 5 years ago

Yay! πŸŽ‰ Tests are passing 😊

Edit: Tests are passing, but too early to celebrate πŸ˜… Still needs some work. Will update here.

Update: Fixed 😊

sglyon commented 5 years ago

Hey @EricForgy thanks for working on ecosystem issues.

I am open to being convinced otherwise, but would lean towards not dropping blink.

I'm curious what PlotlyJS - Blink - WebIO offers over PlotlyBase?

EricForgy commented 5 years ago

Hi @sglyon,

I could similarly be convinced otherwise, but I kind of wanted Figures.jl to be a backend for PlotlyJS.jl.

Blink is a heavy dependency with a nontrivial build process that is not always smooth (at least for me). It's a very minor change to PlotlyJS.jl that will make things better for me. Not sure what else I can say to try to convince you πŸ˜…

EricForgy commented 5 years ago

I'd even be happy to replace all functionality in a self-contained manner within PlotlyJS.jl if that's what you wanted so any functionality currently available via Blink.jl could be done with a browser. This would make PlotlyJS lighter and more robust I think.

sglyon commented 5 years ago

Haha I appreciate the honesty!

I'm still curious what what PlotlyJS - Blink - WebIO offers over PlotlyBase. Could you elaborate on that?

EricForgy commented 5 years ago

Hi @sgylon πŸ‘‹

I'm still curious what what PlotlyJS - Blink - WebIO offers over PlotlyBase. Could you elaborate on that?

What I was hoping for was more like PlotlyJS - Blink, which is kind of like PlotlyBase + WebIO. The important thing for me was to just removing the Blink dependence. It would be much easier to remove Blink from PlotlyJS than added WebIO to PlotlyBase πŸ˜…

Anyway, that is past tense 😊

You probably remember in the early days when Plotly first open-sourced plotly.js, three separate Plotly packages appeared. Mine was more of a simple websocket bridge between Julia and browser and you just send JS across and it gets evaled in the browser and sends callbacks etc back to Julia. It was simple and worked, but it was more general than just for plotly.js, so that became Pages.jl.

Anyway, in the last couple of days, I ended up just writing my own Plotly.jl package that uses Pages.jl. It was a bit painful because I manually went through nearly all of the JS objects in plotly.js and created Julia versions of them including validation. So now, validation of parameters happens on the Julia side and it is nearly 100% faithful to the plotly.js API.

Similar to PlotlyBase, it generates JSON objects that get sent to the browser, e.g.

julia> s = Plotly.Scatter(x=[1,2,3],marker=Plotly.Marker(symbol="circle-open"))
{"marker":{"symbol":"circle-open"},"x":[1.0,2.0,3.0]}

However, like I said, it has a lot of internal validation and it only generates JSON for variations compared to a default object, e.g.

julia> s = Plotly.Scatter(x=[1,2,3],orientation="v",marker=Plotly.Marker(symbol="circle"))
{"x":[1.0,2.0,3.0]}

Since "v" is the default orientation and "circle" is the default marker symbol, they do not get included in the JSON output.

If you try to give it an invalid input, it will complain, e.g.

julia> s = Plotly.Scatter(x=[1,2,3],orientation="w")
ERROR: Orientation: "w" not found in ("v", "h")
robsmith11 commented 1 year ago

Any update on this?

I just tried https://github.com/JuliaPlots/PlotlyJS.jl/pull/451 and it works great. It speeds up loading PlotlyJS and it also works around an issue I've been having getting Mux installed. It seems reasonable to me that users who want Blink can install it separately.