GenieFramework / Stipple.jl

The reactive UI library for interactive data applications with pure Julia.
MIT License
323 stars 27 forks source link

Model design questions #38

Closed yakir12 closed 1 year ago

yakir12 commented 3 years ago

This is a placeholder for some questions about the design of a model of a dashboard.

Why does it have to be mutable? If every parameter is an Observable then there is no need to mutate the parameter (because the parameter is already a container).

How do we best specify "inner piping" of the model? While most parameters are connected directly to some UI, some are defined as a result of one or more parameters but are not directly affected by a UI. For instance the path of a save file that is composed of two inputs, or a label that depends on the state of one of the parameters, etc. I currently define these relationships after creating an instance of the model, but it would have been better to define these inside the constructor itself. Finally, these parameters that are not directly connected to a UI, they do not necessarily need to be an Observable -- when they change nothing else needs to immediately happen. So they can either be a Ref or indeed, the model should be mutable.

essenciary commented 3 years ago

@yakir12 Thanks for setting this up, it's good to have it open for discussions.

My general opinion is that it's too early and we shouldn't optimize prematurely for any use cases - but rather build the tools, let the users build, and see what design patterns and best practices emerge.

To answer some of your questions:

yakir12 commented 3 years ago

Thanks @essenciary, this indeed answers my questions.

As to the inner pipings: my idea was to use the inner constructor methods of the model struct. But maybe there is a straightforward way to couple the inner constructor to Vue's computed properties?

One more question, I tried and failed to test and see if an instance of the model type is created per client, or do clients share the same instance. Which is it?

hhaensel commented 3 years ago

One more question, I tried and failed to test and see if an instance of the model type is created per client, or do clients share the same instance. Which is it?

It is how you define it. You can create a model as a function of the client id. But you can also share it, if you chose to identify the user by some other mechanism. Have a look at the MultiUser example, where I chose the browser type to define which model instance is used.

hhaensel commented 3 years ago

As to the inner pipings: my idea was to use the inner constructor methods of the model struct. But maybe there is a straightforward way to couple the inner constructor to Vue's computed properties?

I recently added js_watch() and js_computed() to do allow definitions of such properties and methods. It would be great, indeed, if we had types that link to these computed properties.

hhaensel commented 3 years ago

Yesterday evening, I had the idea to write @kwredef as a replacement of @kwdef during development. It allows you to redefine the model without restarting Julia. It's in master now!

macro kwredef(expr)
  expr = macroexpand(__module__, expr) # to expand @static
  expr isa Expr && expr.head === :struct || error("Invalid usage of @kwdef")
  expr = expr::Expr

  t = expr.args; n = 2
  T = expr.args[2]
  if T isa Expr && T.head === :<:
      t = T.args; n = 1
      T = T.args[1]
  end
  if T isa Expr && T.head === :curly
      t = T.args; n=1
  end

  T_old = t[n]
  i = 1
  T_new = Symbol("$(T_old)_$i")
  while isdefined(__module__, T_new)
      i += 1
      T_new = Symbol("$(T_old)_$i")
  end
  t[n] = T_new

  quote
      Base.@kwdef $expr
      $(esc(T_old)) = $(esc(T_new))
  end
end

EDIT: There is even a free docstring included 😉 EDIT2: Needed to replace the last part by the original code in @kwdef, due to a scoping issue...

yakir12 commented 3 years ago

Me every time I find a docstring in Stipple: tenor

hhaensel commented 3 years ago

You look much younger on your avatar 🤣

yakir12 commented 3 years ago

(I need to update that...)

essenciary commented 3 years ago

M.@kwdef ...

end

yakir12 commented 3 years ago

This sounds amazing.

As a non-web-dev, any abstraction away from JS you can offer will be a huge gateway for new users. So things like js_computed, and js_watched, sound great, but if there's a way to hide the details in Observables etc then that would allow everyone to join this party.

hhaensel commented 3 years ago
@static const M = isprod() ? Base : Stipple

M.@kwdef ... 

end

In that case, the user would have to care about the definition of M, if i'm correct. Alternatively, we could leave the definition of @kwredef and add

macro kwdef(expr)
  kwdefs = if Genie.Configuration.isprod()
    macroexpand(__module__, :(Base.@kwdef $expr))
  else
    macroexpand(__module__, :(Stipple.@kwredef $expr))
  end
  :( $(esc(kwdefs)) )
end

The question is, should we then export @kwdef? Perhaps not, each user can decide to import it ... @kwredef could be exported without problems.

hhaensel commented 3 years ago

I also propose the following changes:

hhaensel commented 3 years ago

I plan to submit these changes as proposed, but I wonder whether we should import/reexport StippleUI by default. Alternatively, we could reexport Stipple, when using StippleUI. No one will understand, why btn() will define a quasar component but span() will produce an error. - Maybe the second option is the preferred one...

@essenciary: I saw that you reexported Genie and Genie.Renderer.Html and I think this was the absolutely right decision! 🚀

hhaensel commented 3 years ago
* shared model instance - I was actually thinking that the default setup should be individual model instance per user. The current shared model is not what most users would want/expect.

Could you help me understand how you define a user?

hhaensel commented 3 years ago

I decided to commit my changes as I think they are not breaking. @essenciary please review if you find the time so that we can release a new version. WebCam.jl has beome a nice example for the new features 🚀

EDIT: I have the intention to attach docstrings to each function that I newly create. At least for this commit, i did! Plus some extras 😉

essenciary commented 3 years ago

Cool stuff, thank you!

A user is a session_id - we can drop it in a cookie and/or append it to the URL (to be shared)

hhaensel commented 3 years ago

@essenciary I'm planning to implement the readonly type and some more stuff, but I don't have a clue how to do the wrapping. Somehow we would need to subtype observables. Could you give me demo to start?

EDIT: Would this be the right way to start?

using Observables
struct Readonly{T} <: Observables.AbstractObservable{T}
    o::Observable{T}
end
import Base.getindex
import Base.setindex
import Observables.observe

getindex(v::Readonly{T}, args...) where T = getindex(v.o, args...)
setindex(v::Readonly{T}, args...) where T = setindex(v.o, args...)
observe(v::Readonly{T}, args...; kwargs...) where T = observe(v.o, args...; kwargs...)

Readonly(v) = Readonly(Observable(v))
essenciary commented 3 years ago

@hhaensel that looks good - although if we go the path of wrapping an observable, I suggest we convert our Reactive into something like your code plus an additional readonly property. This way we only have one type, which can be readonly.

Something in the line of

struct Reactive{T}  
    o::Observable{T}
    readonly::Bool
end

# forward getindex, setindex, etc to wrapped Observable, per your code

Reactive(v) = Reactive(Observable(v), false)

This will also allow us to extend the API with additional fields in the future, if necessary.

What do you think?

hhaensel commented 3 years ago

That's also possible, I would have proposed

const Reactive=Observable.AbsractObservable
const R = Observable.Observable

Stipple.@kwdef mutable struct <: ReactiveModel
    a::R{String} = "I'm bi-directional"
    a_readonly::ReadOnly{String} = "Don't touch me!"
    js_dict::JSFunction{OptsDict} = opts()
    js::JSFunction{JSONText} = JSONText("""() => console.log("Hello World!")""")
end

But I agree the following is more intuitive

const Reactive=Observable.AbsractObservable
const R = Observable.Observable

Stipple.@kwdef mutable struct <: ReactiveModel
    a::R{String} = "I'm bi-directional"
    a_readonly::R{String, :readonly} = "Don't touch me!"
    js_dict::R{OptsDict, :jsfunction} = opts()
    js::R{JSONText, :jsfunction} = JSONText("""() => console.log("Hello World!")""")
end

The jsfunction type is meant for coding functions that are passed via JSON and a corresponding reviver function. The problem is that we need to pass something else to WebChannel than we need for the creation of the Vue instance. So the listener needs to be adapted to broadcast something else than it stores in the field. I have everything in place for that now that I know how to build the wrapper type. Thanks for your input. 😃

hhaensel commented 3 years ago

@essenciary I've pushed a new branch hh-newRevise and a corrseponding demo.

It demonstrates the different use cases of the API. It would be great to hear what you think about that approach. I designed the mode :jsfunction that can take a javascript function in form of a JSONText and parses it to a JSON compatible form. It accepts either JSONText or Dicts with JSONText values as possible types. On the backend a reviver transforms that form back to a js function in the context of the vue element. This is all hidden from the user and taken care of by the push!(model, field::Reactive, ...) method. This mode is, of course, a readonly mode. Readonly parameters are excluded from the watcher list. Combined with the js_watch it is possible to execute functions in the backend.

For testing, open the browser console and watch what happens when you execute the last 4 lines of the demo.

essenciary commented 3 years ago

@hhaensel Fantastic, I'll check it out, thank you!

Quick question: I'm working on adding comments and tests to Stipple so I'm going over all the code in Stipple.jl at the moment. So I went on and specialized some of the js_* methods you added which were defined for Any type of arguments. Then I saw you defined more specialized versions for ReactiveModel which delegated to the Any. Any reason for that? Can we just keep the definition which accepts ReactiveModel?

image

essenciary commented 3 years ago

@hhaensel Also, another question related to the js_* functions - shouldn't these be model aware? I see that they take the model as argument, but it's not used. What happens if there are two Vue apps on the same page?

The js_* methods seem similar to register_components which is per model. So that if we have two models (with two Vue apps on the same page) each Vue app has its own components (basically the JS code is injected into the corresponding Vue app).

So actually maybe these should be specialized for the type of model, eg:

function js_created(m::Type{T})::String where {T<:ReactiveModel}
essenciary commented 3 years ago

@hhaensel I'm done with the Stipple.jl file - I'm planning on merging after adding tests, but if you want to take a look (the only major change is that I removed the js_* methods defined for Any)

https://github.com/GenieFramework/Stipple.jl/compare/comments-and-tests?expand=1

hhaensel commented 3 years ago

I can only have a look tonight...

hhaensel commented 3 years ago

I commented already in the commit, but I repeat it here. That all looks good to me, except that I have troubles with @static in line 533. And I submitted a fix for @iif. Do we need the @static? - Do you have an idea why this fails? I'm on 1.6-rc3, Windows 10, 64bit.

hhaensel commented 3 years ago

And a big compliment for all the documentation! 🚀 🚀 🚀

hhaensel commented 3 years ago

One more question on the macros @iif etc. Why don't we call them @if, @else, @elseif ?

essenciary commented 3 years ago

Thank you! I tried to make them as @if @else but it didn't work as these are language keywords. Eg macro if is a syntax error. If you know any trick to make this work that'd be awesome.

hhaensel commented 3 years ago

Hm, I think it is not possible ... Three ways out: 1) use @If, @Else, @Elseif, but Jive.jl uses and exports @If already 2) use @v_if, @v_else, @v_else_if 3) I submitted a fix to StippleUI to support keyarg syntax: v__if = :condition etc.

I personally would suggest to provide solutions 2) and 3)

essenciary commented 3 years ago

@hhaensel I think it's confusing to use uppercase constructs. I'm not a fan of @v_* as it has no real meaning. We're basically leaking the underlying Vue integration and making that a part of our API, when Stipple's job is to abstract away as much of the JS code as possible. Lacking any better option, I'd say we can leave these as they are (except working :D )

hhaensel commented 3 years ago

I'm not a fan of @v_* as it has no real meaning.

You could understand it as visible-if 😉 Or I could offer @showif, @showelse, @showelseif / @show_if, @show_else, @show_else_if

essenciary commented 3 years ago

We also need to support v-for so it would be good to come up with a standardized naming scheme.

hhaensel commented 3 years ago

Well, that could be @for, couldn't it?

hhaensel commented 3 years ago

I'm not a fan of @v_* as it has no real meaning.

I think vue's naming is not very good here, but we could name it @showifetc. and then @for would be very natural. We eliminate the "v-" and "q-" wherever that is possible. We still support the v__if=:toggle syntax for those who want to stay close with original vue syntax.

yakir12 commented 3 years ago

Most of this is way over my head, and I should therefore stay quite, but one thing that makes sense to me at least, is what Adrian said about abstracting away from JS code. So if there was a way to decouple from the Vue logic as well as syntax then that would be the best way forward. It would be best if the users wouldn't need to learn the subtle differences between how observables work and how Vue arranges methods, created, and watched, for instance.

I guess what I'm asking is: is Vue workings optimal and therefor should be exposed to the user (via macros etc) or is it possible to build a better system that abstracts away all that jazz into something more Julian/friendly?

I apologize if this comment isn't helpful, I'm sure I misunderstood at least some of this stuff.

essenciary commented 3 years ago

@hhaensel for is a keyword as well.

TBH (and I'm being fully subjective here) I find iif (an actual keyword standing for if in other languages) better than showif. The current API is just much shorter. What's the issue with iif els and elsiif?

@yakir12 indeed. As a matter of principle, common tasks should be easy in Stipple without writing JS. Ideally this would cover some 80% of the use cases. Complex tasks (the remaining 20%) should be possible by hooking into the underlying JS layer (by writing JS by hand through the APIs that @hhaensel added).

I can't really see Stipple being JS-library-agnostic (eg, I can't really see a scenario where we'll have say React as a alternative to Vue, Vue is just too coupled into the fabric of Stipple), but still, the reliance on Vue should be just an implementation detail (in the end, for the 80% of the use case, there should be only Julia with a very Julian syntax and workflow).

hhaensel commented 3 years ago

The current API is just much shorter.

Well it is just 3 letters for iff and 4 letters for els and elsiif. But it really tells what it does and it would go in the direction of not rebuilding the vue API. We could also offer @hideif 😉

v-for is indeed a difficult case, @ffor comes to my mind, but to me that's similar to @iif, which I personally don't like too much.

essenciary commented 3 years ago
hhaensel commented 3 years ago

Shall we close this topic and open a new one for the remaining things? Part of this could also go into the Project...

cwiese commented 2 years ago

THis would be a cool feature Computed

struct X <: ReactiveModel
firstname::R{String} = ""
lastname::R{String} = ""
fullname::Computed{R{String},R{String}} = (firstname, lastname) -> "$firstname $lastname"
end
hhaensel commented 2 years ago

If we had a decent manual, I would say RTFM 😉 But now I say, it's already there.

julia> import Stipple.js_computed

help?> js_computed
search: js_computed

  `function js_computed(app::T) where {T<:ReactiveModel}`

  Defines js functions for the computed section of the vue element. These properties are updated every time on of the     
  inner parameters changes its value.

  Example
  –––––––––

  js_computed(app::MyDashboard) = """
    fullName: function () {
      return this.firstName + ' ' + this.lastName
    }
  """
hhaensel commented 2 years ago

We have all the family:

js_computed js_created   js_methods   js_mounted   js_watch
hhaensel commented 2 years ago

One more thought on the proposal

struct X <: ReactiveModel
firstname::R{String} = ""
lastname::R{String} = ""
fullname::Computed{R{String},R{String}} = (firstname, lastname) -> "$firstname $lastname"
end

Did you want to propose to have fullname synchronised automatically? That's, indeed, something, we did not consider up to now. It would be feasible, but I'm not sure whether it is something desirable. In this case I would rather have fullname a regular Reactive variable and implement the computation on the server side.

onany(model.firstname, model.lastname) do firstname, lastname
    model.fullname[] = join([firstname, lastname], " ")
end

@cwiese Can you provide a scenario where client-side calculation is better suited than server-side? We have fine-grained control over watcher on the server-side and client side, so we could implement this even with current means, it would be less elegant, though.

cwiese commented 2 years ago

so far twice I needed to use a observer for a simple derived value example model_label

Stipple.@kwdef mutable struct MainDash <: ReactiveModel
  tab::R{String} = "fitting"
  models::R{Vector{String}} = String[]
  model::R{String} = ""
  model_label::R{String} = "Select Model"
  leftDrawerOpen::R{Bool} = true
  spinner::R{Bool} = false
  simdata::R{Dict} = Dict()
  @mixin DatasetFilter() "dsf_"
  @mixin DatasetFilter() "dsf2_"
  @mixin DatasetFilter() "sf_"
  @mixin DatasetFilter() "sf2_"
  @mixin DatasetSelect() "dss_"
  @mixin SimulationSelect() "ss_"
  @mixin PlotModel() "plot_"
end

    on(dash_model.model) do (_)
        dash_model.model_label[] = length(dash_model.model[])>1 ? dash_model.model[] : "Select Model"
    end

and for length displayed 'len_options'

Stipple.@kwdef mutable struct DatasetSelect <: ReactiveModel
    name::R{Vector{String}} = String[]
    options::R{Vector{String}} = ["n/a"]
    len_options::R{Int} = 20
    options_::Vector{String} = ["n/a"]
    attr_name::R{Vector{String}} = String[]
    attr_options::R{Vector{String}} = ["n/a"]
    attr_options_::Vector{String} = ["n/a"]
end # dss_

because I am using the "prefix" for the model Mixin . I maybe have several "selects" using this model 

  on(getfield(dash_model, Symbol("$(prefix)options"))) do (_)
    setfield(dash_model, Symbol("$(prefix)len_options"), length(getfield(dash_model, Symbol("$(prefix)options"))))
  end