Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit removal of Blink as a dependency #272

Open
EricForgy opened this issue Mar 11, 2019 · 9 comments · May be fixed by #451
Open

Revisit removal of Blink as a dependency #272

EricForgy opened this issue Mar 11, 2019 · 9 comments · May be fixed by #451

Comments

@EricForgy
Copy link
Contributor

EricForgy commented Mar 11, 2019

Hi @sglyon 👋

I raised this issue once here

#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:

  • Not lose any Blink.jl functionality currently in PlotlyJS.jl
  • Use Requires.jl so that functionality only gets loaded if someone independently does using Blink
  • Remove Blink.jl as a dependency of PlotlyJS.jl

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
Copy link
Contributor Author

Oh yeah. I forgot about this too :)

#41 (comment)

@EricForgy
Copy link
Contributor Author

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
Copy link
Contributor Author

EricForgy commented Mar 13, 2019

Yay! 🎉 Tests are passing 😊

Edit: Tests are passing, but too early to celebrate 😅 Still needs some work. Will update here.

Update: Fixed 😊

@sglyon
Copy link
Member

sglyon commented Mar 13, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

sglyon commented Mar 13, 2019

Haha I appreciate the honesty!

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

@EricForgy
Copy link
Contributor Author

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")

@halleysfifthinc halleysfifthinc linked a pull request Nov 1, 2022 that will close this issue
@robsmith11
Copy link

Any update on this?

I just tried #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants