-
Notifications
You must be signed in to change notification settings - Fork 95
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
Maybe some improvment to the tutorials #365
Comments
Also, van.tags should have the concept of an empty node (or conditional node). So the case |
Thanks @X-Ryl669 for the feedback!
It's not possible due to the limit of DOM API, a pass-through container element is recommended (e.g.:
As I mentioned in 1., a pass-through container element is recommended. For your specific use case, you might want to check out
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}, "👎"),
)
}
|
Well, in the example you provided, it does (in the But even if you only had the 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 ?) |
It doesn't. Inside the
|
Yes I understood that. I was talking about the previous line,
Yes. And since there isn't anything depending on Said differently, the example dumb code I've posted above would have detected the reactiveness of the function via Let's say you have a 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. So my pseudo code above would have warned correctly about this incorrect usage for |
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 van.add(document.body, Counter) Then it becomes reactive, since now each time the 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 |
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 However, if you replace the last line with |
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 van.add(document.body, Counter()) To be clear, VanJS encourages its users to enclose the access to state 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 Yes, inside the button({onclick: () => counter.val = counter.val + 1}, "👍") When user clicks the button, Q2: Can I warn the user whenever a state 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:
Thus if some state Last but not the least, for your comments:
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 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 |
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, 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 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 Both have numerous drawback, writing As for Q2, again, I understood that since the beginning.
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 |
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. 1. VanJS encourage its users to enclose the access of state
|
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):
The text was updated successfully, but these errors were encountered: