Open X-Ryl669 opened 2 months ago
Also, van.tags should have the concept of an empty node (or conditional node). So the case if (cond) add(node1) else don't
, could be done with something as simple as return div(cond(condition, span("Condition is true")))
that would produce <div><span>Condition is true</span></div>
or <div></div>
Thanks @X-Ryl669 for the feedback!
- How do I add multiple (reactive) node without a parent node
It's not possible due to the limit of DOM API, a pass-through container element is recommended (e.g.: <span>
or <div>
). We have some notes in "VanJS by Example" page. I added similar notes to the tutorial as well:
- How do I create a (reactive) node those presence depends on a signal / reactive value
As I mentioned in 1., a pass-through container element is recommended. For your specific use case, you might want to check out vanX.list
, which can make the DOM update more efficient.
Is it possible to detect, in debug build, that the arguments to
van.add
are reactive but aren't a function AND are avan.tags
making use of a signal, and thus output an error on the console?
This is probably hard. Sometimes global access to state values is desired. Taking the example in the home page:
const Counter = () => {
const counter = van.state(0)
return span(
"❤️ ", counter, " ",
button({onclick: () => ++counter.val}, "👍"),
button({onclick: () => --counter.val}, "👎"),
)
}
counter.val
is accessed in the click
event handler. The access is not enclosed in any binding function.
The access is not enclosed in any binding function.
Well, in the example you provided, it does (in the span
function).
But even if you only had the onclick
handler it won't be an issue. The issue comes when you read the variable, not when you change it. It's only when you read a variable that the context becomes reactive, IMHO. If the (binding) function doesn't read the variable, we don't care if it's not created as a "reactive" context since it can't change depending on the variable (or, said differently, if the signal changes, the function can't give a different output).
Any global write to a variable (via an on handler or directly in the binding function) doesn't have to be reactive if the function isn't (after all, if it doesn't monitor any reactive variable, who cares if the function is called again if whatever alien variable is modified ?)
Well, in the example you provided, it does (in the
span
function).
It doesn't. Inside the span
function, we're just assigning the function-typed value () => ++counter.val
to the onclick
property. Only after user clicks the button, the function will be called, and it won't be enclosed in any tag function at that time.
The issue comes when you read the variable, not when you change it.
counter.val++
indeed reads the state as it's equivalent to counter.val = counter.val + 1
.
Yes I understood that. I was talking about the previous line,
That code=> "❤️ ", counter, " ",
references (probably read) the counter
signal value to create the final string. And it's the only side effect that's reactive in this function. If it wasn't there, the function wouldn't need to be reactive, since it would only produce the exact same output whatever the state of the counter
signal.
counter.val++ indeed reads the state as it's equivalent to counter.val = counter.val + 1
Yes. And since there isn't anything depending on counter.val
in this function that would impact the outer function, it's perfectly fine if the tracking code wouldn't trigger here.
Said differently, the example dumb code I've posted above would have detected the reactiveness of the function via globalReactiveFound
because of "❤️ ", counter, " ",
.
Let's say you have a Counter2
variable like this:
const counter = van.state(0) // Put outside else it really make no sense for this example
const Counter2 = () => {
return span(
button({onclick: () => ++counter.val}, "👍"),
button({onclick: () => --counter.val}, "👎"),
)
}
(I've removed the reactive part of the original function): my code wouldn't have detected it, as expected, since the only other reactive access is in the handler thus, not called when added to the DOM.
Yet since the function does not access the counter (in the direct body), it won't be an issue since changing the reactive counter
in the handler won't be observable in the Counter2
function
So my pseudo code above would have warned correctly about this incorrect usage for Counter
(if used like van.add(parent, Counter())
and also, not warned uselessly for Counter2
if used the same way.
I'm take a completely more convoluted example with some recursion to show it would still work, I think:
const {button, span, div} = van.tags
// Let's make a state that's modifiable by anyone
const counter = van.state(0)
const Counter = () => {
return span("Counter: " + counter.val)
}
van.add(document.body, button({onclick: ()=>++counter.val}, "Click"))
van.add(document.body, Counter())
This example will not work as expected, since the Counter
function generate a single span with a fixed text.
If you change the last line by
van.add(document.body, Counter)
Then it becomes reactive, since now each time the counter
signal is trigger, the function is re-evaluated/recalled.
In that kind of code, my "reactiveness detection warning" would have triggered (since there's a get access on counter.val
)
Now, let's make it more complex:
const {button, span, div} = van.tags
const counter = van.state(0)
const Counter = () => {
const d = div("CONT")
return div(d, button({onclick: ()=>{++counter.val; van.add(d, "Clicked "+counter.val)}}, "Click me"))
}
van.add(document.body, Counter()) //<= notice the error here
Here, the function Counter isn't reactive anymore (in its main body), yet one of the child node is (it has a onclick handler that depends on counter.val
). The detection code above wouldn't have warned about it as expected because it will work (and it does, clicking on the button changes a node reactively)
Even in the tutorial, there are subtleties. The code you've posted above (the sorting example), isn't how a javascript developer would write it. I think it'll be written more intuitively like this:
const {input, li, option, select, span, ul} = van.tags
const items = van.state("a,b,c"), sortedBy = van.state("Ascending")
const SortedList = () => {
return span(
[...]
// A State-derived child node
sortedBy.val === "Ascending" ? // <= Changed that, it makes no conceptual sense to use a function here
ul(items.val.split(",").sort().map(i => li(i))) :
ul(items.val.split(",").sort().reverse().map(i => li(i))),
)
}
van.add(document.body, SortedList())
Because in usual Javascript you don't write functions when you're evaluating a variable.
But if you write it this way, it's not reactive.
When sortedBy changes, the ul
isn't diff/modified.
However, if you replace the last line with var.add(document.body, SortedList)
(the argument is a function, not the result of calling the function), then it becomes reactive again and works as expected. I cheated a bit here since the state variables are now outside the function to avoid recreating new one each time the function is called.
To summarize, do you propose to add warnings for all calls the reactive values that are called in non-reactive functions?
If you pass a value that's a tag (or an array of tags?) to van.add and the global "is any state variable was read" marker is true, then it's very likely an error so yes, it should warn.
If you pass a function then it's likely ok.
Detecting if a state variable was read is a complex task, prone to error, so it can't be 100% efficient here, but at least it would catch the most basic error cases.
Hmm..., it looks like there are lots of replies when I was out for a walk. Thanks @X-Ryl669 so much for contributing your thoughts!
First, there might be some misunderstanding on how certain examples work in VanJS. Specifically,
span(
"❤️ ", counter, " ",
...
)
is equivalent to (think of it as a syntax sugar):
span(
"❤️ ", () => counter.val, " ",
...
)
Thus the access to counter.val
in enclosed by a binding function in the smallest scope, which is encouraged in VanJS's coding style. So in that sense, the Counter
component is hermetic. Meaning that, you can safely call Counter()
anywhere without enclosing it in a binding function, just like what the code example in the home page does:
van.add(document.body, Counter())
To be clear, VanJS encourages its users to enclose the access to state val
s in binding functions with smallest possible scope. Doing that will ensure the smallest extent of DOM update is performed as a result of a state val
change, thus we will get the best efficiency.
I agree with you that it would be nice to raise some warnings for some error-prone code. But I don't think it's quite feasible for this scenario in reality, just as we can't ask a library to warn whenever there is bug in the client-side code.
I will try to reply some of the questions you raised in previous replies:
Q1: Is there a legitimate use case where the state val
is accessed without being enclosed in any binding function?
Yes, inside the click
event handler of the button (I'm using the de-sugared form to make it clearer):
button({onclick: () => counter.val = counter.val + 1}, "👍")
When user clicks the button, counter.val = counter.val + 1
will be executed, counter.val
is accessed without being enclosed in any binding function.
Q2: Can I warn the user whenever a state val
is accessed outside binding functions whenever van.add
is called?
No, this is not possible, due to the execution order of function calls.
Take the code below as an example:
van.add(document.body, Counter())
If we break it down. The execution order is like that:
Counter()
, saving the result into a temporary variable temp1
.van.add
, passing document.body
and temp1
as its arguments.Thus if some state val
is accessed inside the Counter()
call, it's accessed before van.add
is called, not during val.add
is called.
Last but not the least, for your comments:
Even in the tutorial, there are subtleties. The code you've posted above (the sorting example), isn't how a javascript developer would write it.
I respectively disagree. In JavaScript, like in any other major programming languages, when you write the code like that:
return span(
[...]
// A State-derived child node
sortedBy.val === "Ascending" ? // <= Changed that, it makes no conceptual sense to use a function here
ul(items.val.split(",").sort().map(i => li(i))) :
ul(items.val.split(",").sort().reverse().map(i => li(i))),
)
The code can only be executed when the return
statement is being executed. This is how a program typically works. If you want the code:
sortedBy.val === "Ascending" ? // <= Changed that, it makes no conceptual sense to use a function here
ul(items.val.split(",").sort().map(i => li(i))) :
ul(items.val.split(",").sort().reverse().map(i => li(i)))
to be executed in other occasions, it has to be enclosed in a function. Otherwise, it will be eagerly executed, for only once. There is just no mechanism for other possibilities. There is no magic in VanJS, everything does what a typically program is supposed to do. The reason why the code above is being felt as more intuitive is that the magic part of popular web frameworks such as React (transpiling) distorted the way how a program should be perceived.
Also what I want to add is that I am all for better debuggability in VanJS. Just as recently, we launched VanGraph which can help you visualize the dependencies among states and DOM nodes. I believe some missing dependencies caused by "forgetting to enclose val
access inside binding functions" can be caught by visualizing the dependency graph.
To be clear, VanJS encourages its users to enclose the access to state vals in binding functions with smallest possible scope. Doing that will ensure the smallest extent of DOM update is performed as a result of a state val change, thus we will get the best efficiency.
That links back to the initial point 3 in the issue, an explanation of how data flow (and the implications) with some schematics would be very useful to understand why and how to write code like this.
If I take the example as provided, "❤️ ", counter, " ",
I would expect this to only create a single TextNode in the span containing, for example ❤️ 1
. Thus, I can't imagine why doing "❤️ " + counter.val + " "
in Counter
function would be different from van.js doing the same in the span
function. In terms of DOM update, I mean, it'd be the exact same operation if counter
evolves, you need to replace the textNode with the new one.
Can you provide a counter example where doing this way leads to saving in DOM update ?
As for the question Q1, it was clear to me, since the beginning. And the point I raised is that, since the only way for a "reactive" function (like Counter
) to change the DOM isn't in the (asynchronous) handler functions but in it using a reactive variable/signal in its direct (synchronous) body, it's possible to completely skip reactiveness or recomputing if there's no such use.
Said differently a non reactive function can trigger a change in a state/signal in an handler, it won't have any effect in the function, there's no point in making it a reactive function. However, currently, you can't use a state's .val
in a non function context (out of semantically-useless ()=>
scope) without loosing the reactiveness, unless you make the whole containing function reactive (either by adding it to van as a function, not as the result of the call of the function).
Both have numerous drawback, writing ()=>
for any state variable usage is counter intuitive (or it should be written in bold in the tutorial: Don't do: counter.val = 3, do: ()=>counter.val = 3 in the outer function and explain why so).
If you don't do that (that is, use a state variable like a standard javascript variable), then the outer function must be added as a function to van.add
and not as a call to the function, so van can reevaluate the function each time any of its state variable is modified. You said this is to avoid since it might be less efficient, but an example describing why it's less efficient would be greatly welcomed.
As for Q2, again, I understood that since the beginning. When you say:
Otherwise, it will be eagerly executed, for only once. There is just no mechanism for other possibilities.
I disagree. The whole function can be executed again and the result will differ (look at all the examples I've provided, I stored the function in van.add, not the result of the function). I've tested all of them and all works.
So I'm giving a recipe to vanjs to build what I want (that needs to be evaluated to get all the elements each time a state variable change), while you are giving the result of all the elements with some being dynamic and only those need to be reevaluated when a state variable change. Technically, every tag element is dynamic but some will produce static content when evaluated
I can see the different philosophy here. I've hard time understanding why the latter would be more efficient, in the end, the number of elements to change in the DOM is exactly the same both way.
Maybe the reason is due to some easier algorithm to spot the static from dynamic element so explaining this would be good to understand this technical choice.
I think I've overlearnt all the fancy javascript frameworks since many years and I'm biased in my reading. Maybe I understood the van example code a bit like lit or µhtml html
template code (which are just other fancy way in Ecmascript to call a function). Their paradigm is also to give a recipe/template to the framework that'll build the DOM to match the recipe (and react to change if required when some component of the recipe change).
Thanks @X-Ryl669 for your long reply!
I will try to organize my response in a more structured way to make it easy for us to understand each other.
val
in a binding function with the smallest possible scope. Could you explain why it makes DOM updates more efficient?Let me just use the Counter
example to illustrate, this is the sample code in the home page (in the de-sugared form):
const Counter = () => {
const counter = van.state(0)
return span(
"❤️ ", () => counter.val, " ",
...
)
}
This is the "leaky" implementation we want to compare with. Here I name it as LeakyCounter
because the access to state val
is leaked out of the scope of the component. In other words, LeakyCounter
itself needs to be enclosed in another binding function to be reactive (I hope we can consolidate the terminology for easier discussion 🙂):
const LeakyCounter = () => {
const counter = van.state(0)
return span(
"❤️ ", counter.val, " ",
...
)
}
That is, instead of having van.add(document.body, LeakyCounter())
, we need to have van.add(document.body, () => LeakyCounter())
, or van.add(document.body, LeakyCounter)
for short.
In the Counter
version, when counter.val
is updated, only the localized text node (that just contains the counter number) needs to be updated. However, in the LeakyCounter
version, when counter.val
is updated, the function LeakyCounter
needs to be called again, meaning a few text nodes, 2 button elements, and a parent <span>
element, all need to be reconstructed, and the entire subtree of the original DOM needs to be replaced by all the newly constructed elements.
I hope at this point you can get an idea on why enclosing the access to state val
in the smallest scope would make reactive DOM updates more efficient.
I think some of our previous discussion in this thread focuses on figuring out a way to automatically detect the implementation of "leaky" components and warn users about it. Sadly, in my opinion, I don't think there is a good way to do that. The main difficulty is that, sometimes, accessing the val
of a state outside any binding function is a completely legitimate use case, thus we can't use that as a signal of "leaky" component. If you think you have a good way to detect it, you can share your algorithm here and we will see if it can solve the problem.
I disagree. The whole function can be executed again and the result will differ (look at all the examples I've provided, I stored the function in van.add, not the result of the function).
I'm sorry my previous reply was too concise so it's probably slightly inaccurate. What I wanted to say is, for the code:
return span(
...
// A State-derived child node
sortedBy.val === "Ascending" ? // <= Changed that, it makes no conceptual sense to use a function here
ul(items.val.split(",").sort().map(i => li(i))) :
ul(items.val.split(",").sort().reverse().map(i => li(i))),
)
The code will only be executed eagerly and only once whenever the enclosing function is called. So with typical programming intuition (not with the intuition of the magical React JSX), the UI elements won't be reactive unless the enclosing function is reactive to sortedBy
state. In a shorter example:
const LeakyCounter = () => {
const counter = van.state(0)
return span(
"❤️ ", counter.val, " ",
...
)
}
van.add(document.body, LeakyCounter())
it will be intuitive to know the DOM tree won't be reactive when counter
changes, as there is just no mechanism for counter.val
to be read again in this implementation.
As an analogy, we actually have similar intuition for event handlers of DOM elements. We actually have the intuition to know the handler should be implemented like this:
button({onclick: () => ++counter.val}, ...)
instead of:
button({onclick: ++counter.val}, ...)
because we would know that in the latter implementation, there is just no mechanism that ++counter.val
can be executed multiple times (except in the case where the entire expression button({onclick: ++counter.val}, ...)
is being executed again).
or it should be written in bold in the tutorial: Don't do: counter.val = 3, do: ()=>counter.val = 3 in the outer function and explain why so
This is obviously a misunderstanding of VanJS. For setting the val
property, there is no point to enclose it in a function.
The tutorials are good, but there are few points that aren't clear and that could be improved, IMHO.
For example:
For 1, I'm doing this, don't know if it's what recommended:
Tutorials always show this, but it's not required
Yet, sometimes returning an array doesn't work (see point 2 below)
For 2, I find it's extremely hard to achieve in the current state of the library. For example, this doesn't work
The workaround is like this:
BTW, I know that in the former case, the function
DerivedState()
is called once and can't be recalled on reactive change thus it can be changed to()=>DerivedState()
or simplyDerivedState
, but it's very hard to understand or spot the error. At least the documentation should list it.Is it possible to detect, in debug build, that the arguments to
van.add
are reactive but aren't a function AND are avan.tags making use of a signal
, and thus output an error on the console? I couldn't think of a case where these conditions would work out well.Something like this (very poor man detection method of a reactive signal call inside a function):