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

Maybe some improvment to the tutorials #365

Open
X-Ryl669 opened this issue Sep 9, 2024 · 13 comments
Open

Maybe some improvment to the tutorials #365

X-Ryl669 opened this issue Sep 9, 2024 · 13 comments

Comments

@X-Ryl669
Copy link

X-Ryl669 commented Sep 9, 2024

The tutorials are good, but there are few points that aren't clear and that could be improved, IMHO.

For example:

  1. How do I add multiple (reactive) node without a parent node
  2. How do I create a (reactive) node those presence depends on a signal / reactive value
  3. How the data flow into the system (and when)

For 1, I'm doing this, don't know if it's what recommended:

const Func = () => {
	return [span("A"), span("list"), span("of"), span("nodes")]
}

van.add(document.body, Func())

Tutorials always show this, but it's not required

const Func = () => {
	return div( // <= Useless container 
                span("A"), span("list"), span("of"), span("nodes"))
}

van.add(document.body, Func())

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

let a = van.state(0)

// Doesn't work as expected, won't be changed when a.val is modified
const DerivedState = () => {
  if (a.val)
	return [span("A"), span("list"), span("of"), span("nodes")]
  else 
	return [span("A"), span("list"), span("nodes")]
}

van.add(document.body, DerivedState())
a.val = 1

The workaround is like this:

let a = van.state(0)

// An effect must be created here else, it isn't reflected in the DOM
const der = van.derive(() => a.val ? div(span("A"), span("list"), span("of"), span("nodes")) : div(span("A"), span("list"), span("nodes")))

// This doesn't work either, so we're back to the useless container
//const der = van.derive(() => a.val ? [span("A"), span("list"), span("of"), span("nodes")] : [span("A"), span("list"), span("nodes"))]


van.add(document.body, der)

// Trigger change to check if DOM is modified
a.val = 1

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 simply DerivedState, 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 a van.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):

let globalReactiveFound = 0

class AProx // Mimic state interface
{
    constructor(e = 0) { this.e = e }
    set val(e) { this.e = e; globalReactiveFound = 1 }
    get val() { globalReactiveFound = 1; return this.e }
}

var b = new AProx(0)
function something() {
    console.log(b.val); // Reactive access
}

const va = {
  add: function(e) { if (globalReactiveFound) console.error("e is reactive but won't react to change"); globalReactiveFound = 0 }
}
va.add(something()) // Here, calling the function will change the flag thus is detectable in va.add 
@X-Ryl669
Copy link
Author

X-Ryl669 commented Sep 9, 2024

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>

@Tao-VanJS
Copy link
Member

Thanks @X-Ryl669 for the feedback!

  1. 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:

scrnli_9_9_2024_11-08-36 AM

  1. 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 a van.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.

@X-Ryl669
Copy link
Author

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

@Tao-VanJS
Copy link
Member

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.

@X-Ryl669
Copy link
Author

X-Ryl669 commented Sep 10, 2024

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.

@X-Ryl669
Copy link
Author

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)

@X-Ryl669
Copy link
Author

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.

@sirenkovladd
Copy link
Contributor

To summarize, do you propose to add warnings for all calls the reactive values that are called in non-reactive functions?

@X-Ryl669
Copy link
Author

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.

@Tao-VanJS
Copy link
Member

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 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.

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:

  1. Call Counter(), saving the result into a temporary variable temp1.
  2. Call 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.

@Tao-VanJS
Copy link
Member

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.

@X-Ryl669
Copy link
Author

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

@Tao-VanJS
Copy link
Member

Tao-VanJS commented Sep 11, 2024

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 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.

2. Is there a way to reliably detect "leaky" components?

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.

3. About programming intuition: why did I say frameworks like React might have distorted the way how a program should be perceived?

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.

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

No branches or pull requests

3 participants