[WIP] Enormous rework of (mostly frontend) code.

[twavv]

@shashi

I did a lot of stuff.

I tested this with Mux (and only implemented a Mux provider) and want to do more stuff with better integration with IJulia but I think this represents a better way of doing things as far as client side code is concerned.

Questions/comments/concerns appreciated. I'd really like to get something like this merged.

My own notes:

• I've injected some of my own frontend opinions; I think the biggest one was just not querying things by id since id should be globally unique in a document (I think I used a class name but we could also query on some data-* attribute like node.querySelector('*[data-webio-scope="scope-id"]')).
• I think the command used to update observables should be changed (I made a note of this in the message.ts file).
• I ended up playing around with the internal WebIO.render pathways of the Julia code and ended up introducing some stuff that I'm personally not fond of and would want to refactor (e.g. renderScopePreferJSON). The reason this is so ugly right now is because I was just having a hard time figuring out the complex ways in which render is recursively called (and I wanted to get rid of <script> tags that render things that then have <script> tags...)
• The escaping is now much simpler since all the data is passed as JSON. Escaping </script> simply becomes escaping all / characters (which is allowed since all / characters appear inside of strings and \/ is interpereted as just /).

This will also require a very slight modification to JSExpr to not use a global WebIO instance. I modified JSExpr locally in two places and everything just worked™:

 function obs_get_expr(x)
     # empty [], special case to get value from an Observable
-    F(["WebIO.getval(", jsexpr(x), ")"])
+    F(["_webIOScope.getObservableValue(", jsexpr(x), ")"])
 end
 
 function obs_set_expr(x, val)
     # empty [], special case to get value from an Observable
-    F(["WebIO.setval(", jsexpr_joined([x, val]), ")"])
+    F(["_webIOScope.setObservableValue(", jsexpr_joined([x, val]), ")"])
 end

(_webIOScope is defined in the context where the function is evaluated).

When done, will close #203, #8. Supersedes #201.
JupyterLab plugin (which I'm more than happy to write/maintain) will close #139.

Again, I'm very open to comments and opinions and changing anything.

[twavv]

I wrote some Blink stuff but it's not quite working; when a WebIO Node is rendered via body!, nothing shows up. And I can't seem to pull up the Blink/Electron DevTools to debug. I asked in the Blink repo how to do that but if anyone knows... 😸

update: it's Blink.opentools(w::Window) fwiw

[SimonDanisch]

Great work!
The Julia code changes seem pretty reasonable - would it be possible to put those in a different PR to make things a bit more incremental?
I have the suspicion, that no one here will be able to review this large diff thoroughly in a reasonable time frame - and I wouldn't want your PR to go stale!

[shashi]

This is pretty awesome! I'll review it soon. Thanks!!

[twavv]

@SimonDanisch The Julia changes are relatively minor and aren't compatible with the previous version of the frontend code, unfortunately, so I don't think it makes much sense to separate them.

I'm also gonna fix up Blink then work on a JupyterLab extension. I'd like to also make a notebook extension which would increase stability of WebIO across page reloads but the current stuff I have implemented should (need to confirm!) work more-or-less as it does now (with the same level of flakiness-on-reload).

[piever]

Thanks a lot! This is a lot of work.

I'm a bit confused as there are in simultaneous this PR and #208 which both seems to be ways to get WebIO to work more reliably: are the two PRs orthogonal or does this one make #208 unnecessary?

I'd tend to agree with Simon that this is very hard to review, but I can definitely check out the PR and see if all the various Interact interfaces still work with this.

Are the tests failing only because or Blink?

Fixing the page reload stuff is definitely very important, there were issues about it in Interact as well (see JuliaGizmos/Interact.jl#250), especially since this is a regression compared to the pre-WebIO implementation of Interact.

[twavv]

Also pending this IJulia pull request.

[twavv]

@piever

Thanks a lot! This is a lot of work.

I'm a bit confused as there are in simultaneous this PR and #208 which both seems to be ways to get WebIO to work more reliably: are the two PRs orthogonal or does this one make #208 unnecessary?

I haven't looked too hard at #208 , but my first impression is that this pull request would make it unnecessary.

I'd tend to agree with Simon that this is very hard to review, but I can definitely check out the PR and see if all the various Interact interfaces still work with this.
Are the tests failing only because or Blink?

I'm not sure why blink is failing on Travis (I'll look into it more in a bit) because it works for me™. I haven't really started the Jupyter Notebook integration yet, but I think that should be just a minor change (I think just having using Interact point to the new JS bundle). I think it should be somewhat more stable than what exists now, but I think writing a notebook extension would be the best way to have ultimate stability (and is also 100% required for JupyterLab).

And thanks for the support 😂.

[twavv]

I'm actually convinced that Blink isn't working as expected, at least when it comes to headless/containerized environments (which includes Travis).

I tried (using the official julia:0.7 image with the current, unmodified versions of Blink and WebIO) to do the following:

Pkg.add("Blink")
using Blink
Blink.AtomShell.install()
w = Window(Dict(:show => false))

and got

ERROR: IOError: connect: connection refused (ECONNREFUSED)
Stacktrace:
 [1] try_yieldto(::typeof(Base.ensure_rescheduled), ::Base.RefValue{Task}) at ./event.jl:196
 [2] wait() at ./event.jl:255
 [3] wait(::Condition) at ./event.jl:46
 [4] stream_wait(::Sockets.TCPSocket, ::Condition) at ./stream.jl:47
 [5] wait_connected(::Sockets.TCPSocket) at ./stream.jl:263
 [6] connect(::Sockets.TCPSocket, ::Sockets.IPv4, ::Int64) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/Sockets/src/Sockets.jl:444
 [7] connect(::Sockets.IPv4, ::Int64) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/Sockets/src/Sockets.jl:428
 [8] #try_connect#5(::Float64, ::Int64, ::Function, ::Sockets.IPv4, ::Vararg{Any,N} where N) at /root/.julia/packages/Blink/vKGzM/src/AtomShell/process.jl:71
 [9] try_connect at /root/.julia/packages/Blink/vKGzM/src/AtomShell/process.jl:69 [inlined]
 [10] #init#6(::Bool, ::Function) at /root/.julia/packages/Blink/vKGzM/src/AtomShell/process.jl:85
 [11] #init at ./none:0 [inlined]
 [12] #shell#9(::Bool, ::Function) at /root/.julia/packages/Blink/vKGzM/src/AtomShell/process.jl:123
 [13] shell at /root/.julia/packages/Blink/vKGzM/src/AtomShell/process.jl:121 [inlined]
 [14] Window(::Dict{Symbol,Bool}) at /root/.julia/packages/Blink/vKGzM/src/AtomShell/window.jl:38
 [15] top-level scope at none:0

Probably related to this Blink.jl issue.

[twavv]

Ah, okay, I figured out all of my woes with Blink.jl (it's definitely not Blink's fault, no matter how much I wanted it to be...).

The issue is that it's passing on my machine™ because I have my own modified (as described above) version of JSExpr. It seems kind of weird that testing WebIO requires JSExpr considering how JSExpr depends on WebIO, so unless anyone has enormous objections, I'm going to remove the JSExpr testing dependency. This should also make it easier to update WebIO and JSExpr independently in the future.

[piever]

Make sure to test Interact widgets before merging this: I see a lot of regressions. In Blink I can't show simple sliders:

julia> using Blink

julia> using Interact

julia> w = Window();

julia> body!(w, slider(1:100))

console error:

Uncaught (in promise) Error: not implemented
    at importResource (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:16878:13)
    at Array.map (<anonymous>)
    at _callee2$ (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:16855:75)
    at tryCatch (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:12751:17)
    at Generator.invoke [as _invoke] (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:12978:22)
    at Generator.prototype.(anonymous function) [as next] (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:12804:21)
    at http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:16718:67
    at new Promise (<anonymous>)
    at ../../webio/src/imports.ts.__awaiter (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:16695:10)
    at importAsyncBlock (http://127.0.0.1:6714/assetserver/c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:16847:10)
2c216e54911b0baacc0e30c0e7d3182f34c1d7bc8-blink.js:sourcemap:16451 Dispatching command "Basics.eval" not implemented.

whereas in Jupyter notebook the slider shows up (but not the value on the right), but rangeslider doesn't appear.

[twavv]

@piever Whoops, my intention was not to suggest that this was really ready for testing (but it's getting closer and closer!).

My only remaining concern is... is rendering observables that contain DOM nodes a necessity (also ping @shashi and @SimonDanisch on this)? It's kind of hard to do this without a single WebIO global which is one of the reasons I made this PR. I see that Interact does this - I'm not sure if it's on purpose or not.

[piever]

If you want to "replace a DOM node" dynamically in your app, the simplest solution is to put it in Observable. Say for example that I want to render node1 if the user sets a checkbox to true and node2 otherwise. In some cases I guess one can render all of them from the start and only set one of them to visible, but in general the Observable{Node} technique seems more powerful.

[twavv]

Yeah, that's kind of what I thought (/was afraid) you were going to say.

So then I think the best attack strategy might be to have Observables be Nodes (in terms of the way that things are presented to the frontend code when WebIO.render'd).

So say you have this Julia code.

w = Scope()
counter = Observable(w, "counter", 0)
w(dom"div"(
    counter,
    dom"button"(
        "increment"
        ; events=Dict("click" => (@js function() $counter[] = $counter[] + 1 end))
    )
))

Currently, this is "transformed" into something like this node representation.

Scope (w)
  - dom"div"
      - Scope (contains counter, has js listener for when counter updates)
          - <pre>0</pre>
      - dom"button"

What happens then is that when an observable appears as a child of some node, it's set up with its own scope whose dom node is the html representation of that observable. This is tricky because then the HTML representation will be injected and will then try to use the WebIO global, which is bad news bears.

So maybe we could have an Observable be rendered as a direct child of a node (without forcing it to be enclosed within its own scope). This is the main point of what I'm trying to say in this long rambling comment.

The only issue I could foresee with this is that there would be issues if the observable rendered is not countained within the closest-ancestor scope. This is already an issue (on the current master of WebIO) if you try to use an observable within javascript while inside of a scope that doesn't contain that observable, and the simplest solution is to just attach all your observables to the same scope. However, what's not currently an issue (and what might become an issue) is that if you just render an observable as the child of a node, it's placed within it's own isolated scope that doesn't depend on whether-or-not that observable is within the ancestor scope.

So, question for @shashi (or anyone else who has thoughts/question):

• Does what I said above make sense (in the sense that you know what I'm talking about)?
• Is that an issue?

It seems like the "fix" would have to be something involving recursively finding observables which might not be that big of a deal...

function find_observables(node::Node)::Vector{Observable}
  observables = Vector{Observable}()
  for child in node.children
    if isa(child, Observable)
      push!(observables, child)
    elseif isa(child, Node)
      append!(observables, find_observables(child)
    end
  end
end

then attach all of the "found" observables to the scope when WebIO.render-ing it (though I'm not sure how that interacts with observable naming and whatnot).

So, assuming the above is not an issue or is taken care of at the very least, this is what I think would be the "optimal" solution.

w = Scope()
nodes = [dom"div"("hello"), dom"div"("goodbye")]
nodeobs = Observable(w, "node", rand(nodes))
w(dom"div"(
    dom"button"(
        "change the node"
        ; events=Dict("onclick" => (@js ...)) 
    ),
    nodeobs
)

would generate the following "node tree":

Scope (w)
- dom"div"
  - dom"button"
  - Observable
    - dom"div"("hello")

(whereas before, the dom"div"("hello") would have been rendered as a giant WebIO.mount(...) string before instead of just containing the JSON representation of the node).

tldr

I hope the above made sense.

Current Issue

When an observable that contains a node is rendered, that observable then contains the big <script>WebIO.mount(...);</script> thing which is not ideal (and won't work in Jupyter right now because it contains guards to not call WebIO.mount if it detects that the nbextension is installed).

Proposed "Fix"

Modify the observable to contain a JSON representation of the node and don't wrap it in it's own scope.

Addendum

We'd need to, I think, modify every rendered observable to become an observable of a node. For example,

w = Scope()
counter = Observable(w, "counter", 0)
WebIO.render(counter)

would then render a scope (w) that contains an observable child (rendered_observable) that contains a node (e.g. dom"pre"("0")). We'd set up a listener for counter that's like

map!(x -> render_as_node(x), rendered_observable, counter)

[shashi]

 Scope (w)
  - dom"div"
      - Scope (contains counter, has js listener for when counter updates)
          - <pre>0</pre>
      - dom"button"

This is of course the easiest and most natural way to implement this. What we could do (I think this is what you suggest) is to make the underlying implementation morally better than .innerHTML = "<script>WebIO.mount(...)</script>". Probably by calling mount on the object to render somehow directly.

if you try to use an observable within javascript while inside of a scope that doesn't contain that observable

I don't think this is the case... Every observable is identified by the (scope id, observable name) pair (see @js $ob[] for the JSON) no matter which scope it is embedded in. this in an observable's handlers will be the scope it is attached to...

Addendum

We already do this right?

[shashi]

Have you seen microsoft/TypeScript#21922 ?

[shashi]

Also any reason we need to keep these in the struct here?

[twavv]

I'm not sure what exactly you're referencing with the TypeScript issue. Do you just mean that we should use EventListener rather than EventLIstenerOrEventListenerObject?

And are you asking why we need to keep the event listeners in the object? I think it's so that we can .removeEventListener them if they change.

[shashi]

I looked up that type but it seems it got removed in Feb? We should probably use whatever alternative if it works.

I think it's so that we can .removeEventListener them if they change.

Ok I see, but I don't think we have a way of doing that (at least from Julia).

[twavv]

🤷‍♀️ I just copied that functionality from the current/master version of the frontend code. I changed the type to EventListener (will push soonish).

[shashi]

Why do we need IFrame here? I think there was an example of a different package implementing its own NodeType. and extending WebIO.NodeTypes[Foo] = {create:...}, I don't know if it still does that, but hypothetically someone could want to do that. Why constrain this?

[twavv]

Well the original reason was because iframes just have some different considerations than regular DOM nodes. In particular, I moved all of the iframe setup code out of iframe.jl and into the frontend.

I can modify things so that the construction of nodes is extensible though.

[shashi]

I figured out what you did, it looks good.

Yeah, would be great to make nodes extensible...

[twavv]

After that push (bbda73e: "Use global scope resolution of observables."), I believe WebIO should work with the older (aka not-modified-by-me) version of JSExpr as well.

[piever]

A couple of feedback from the Interact side:

1. this somehow breaks tabulator from InteractBase, say:

 tabulator(OrderedDict("a" => 1, "b" => 2, index = 1)

Now when clicking on different buttons the value on screen (1 or 2) doesn't get updated.

2. This is something that has bugged me for a while (not a regression introduced here, but somehow I'm hoping that this PR could fis this). If this is totally unrelated, sorry for the noise!

There are some strange issues when rerendering widgets. If I start with:

using Blink
w = Window()
v = Observable(20)
s = slider(1:100; value = v)
body!(w, s)

the slider marks 20. If I move to another number, the s object gets modified, but on rerendering:

body!(w, s)

the displayed value goes back to 20 (even though s[] correctly gives the updated number).

[shashi]

If I move to another number, the s object gets modified

Strange. Do you mean you're calling body!(w, s) after setting s[] = 25?

What happens if you do observe(s)[] = 25 and then render?

[piever]

If I move to another number, the s object gets modified

Strange. Do you mean you're calling body!(w, s) after setting s[] = 25?

What happens if you do observe(s)[] = 25 and then render?

Same effect, it still re-renders to the original value: no idea how that's possible...

[twavv]

I think that's a Julia side fix. The _setup_scope command should cause Julia to send the current value of all the observables within that scope to the client.

[twavv]

@piever
The issue is that somehow, something within the tabulator widget is getting WebIO.rendered'd before it gets to the final before-output stage, so it contains a nested <script>WebIO.render(...);</script> which doesn't work since we let Jupyter invoke WebIO.render (that way it can use the correct instance of WebIO).

[piever]

Is it something that I should fix on the Interact side or there is a way to fix it from here?

[twavv]

I looked into it a little bit more and it seems(?) that it's happening because you have an observable that contains a scope. I can continue looking into it a bit later today I think.

[piever]

it seems(?) that it's happening because you have an observable that contains a scope.

It's kind of crucial that this works though, because every Interact widget is its own scope and it's quite common to put them inside Observable to be able to update the structure of the UI dynamically.

[twavv]

@piever Try now.

There were two factors that were inhibiting things from working.

• Scopes within observables weren't being rendered (first commit, Render scopes within observables.)
• Observables weren't being locally synced -- what I refer to in the code I wrote as "reconciliation" (second commit, Implement observable reconciliation.)

The brilliant and wonderful @shashi pointed the latter out to me a while ago and, as the devoted contributor I strive to be, I promptly forgot.

[piever]

Thanks! That fixes the example above, I think more complex cases are still broken, will try to come up with a MWE soon.

[twavv]

Current status:

• Fix display(...) in Jupyter (this is an issue with the extension code and probably something to do with internal differences between the notebook's handling of display_data and execute_result)
• Copy Delay _setup_scope until after onimport #217
• Implement a setAssetPrefix function to set base url for assets (this is different from setbaseurl! and is needed for JupyterHub)
• Implement a JupyterHub plugin

[twavv]

@piever I think everything is fixed. The Interact demo notebooks render exactly the same in pre- and post-overhaul WebIO. I think we (@shashi) want to merge this soon so if you could take a look I'd really appreciate it.

[piever]

EDIT: I've edited below as I found a much simpler MWE...

I still see some issues with Interact in more complex scenarios.

julia> using Blink, Interact

julia> w = Window()

julia> ui = tabulator(OrderedDict("a" => button(), "b" => button()));

julia> body!(w, ui)

Basically here I'm doing a tabulator whose items are also widgets (each has its own scope) and somehow it seems that both are displayed one on top of another instead. If that can be of any help, tabulator works by rendering all items but only setting one to visible.

[piever]

As a double check, I would also try the more complex (depending on TableWidgets):

julia> using Blink, Interact, TableWidgets, DataFrames

julia> df = DataFrame(x = 1:10);

julia> w = Window()

julia> wdg1 = addfilter(df);

julia> wdg2 = dataeditor(df);

julia> ui = tabulator(OrderedDict("a" => wdg1, "b" => wdg2));

julia> body!(w, ui)

before merging as it seems to fail in a slightly different way than the buttons (here both options are always displayed one on top of the other - with the buttons instead if I click on "b" the first button disappears as it should).

[twavv]

@piever I think it works for me now. The issue was BIZARRE; it's not that important but...

I was looking at the importsLoaded handler (which executes all the Knockout setup code) and my code (falsely) assumed it was a string (when it's actually a list of strings). So it did

// Code operated under assumption that this was a string, not array
const importsLoadedSource = ["function() { ... }"];
importsLoaded = eval(`(${importsLoadedSource})`);
importsLoaded(dependencies);

which caused all kinds of weird nondeterminism that I admit I do not understand.

[piever]

Great job! It seems like even pretty complex GUIs work reliably now (actually, more reliably than they used to...). Maybe this can be merged soonish and the Jupyter plugin happen in a separate PR? The diff already looks massive...

[shashi]

🎉 👏 🍰 🍻