ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
873 stars 124 forks source link

Alternative to use directive syntax #33

Closed Exelord closed 3 years ago

Exelord commented 3 years ago

Hey Ryan :)

I really like the direction you are moving to with dom-expression. The latest feature use directive is very similar to modifiers known from Swift UI or Ember.js. You literally read in my mind with that as I was going to fork the repo and implement it this week.

I was wondering, would you be keen on exposing an alternative syntax? The current one is not fully JSX and types friendly. Have you considered a syntax like this?

<button $={[on('click', clickFn)]}>Click Me</button>

the $ is one of not reserved and allowed by JSX. Also passing the modifiers as an array, makes this syntax fully supported by typescript and JSX.

the example on function would generate a new function that will be used exactly like the current use:on directive. I am really curious about your opinion on that.

Exelord commented 3 years ago

Also what do you think about wrapping the instruction execution with some wrapper?

modifier(on(() => true))

That will allow the libs to wrap it with the reactivity layer.

ryansolid commented 3 years ago

Relevant links: I've mentioned it a couple times before but I think this is the most recent: https://github.com/ryansolid/solid/issues/207 And this is basically the catalyst to this whole thing: https://github.com/microsoft/TypeScript/pull/37421 Syntax inspired by Svelte: https://svelte.dev/tutorial/actions

TypeScript 4.2 is going to finally ship with support for JSX namespaces. I've been waiting for this a long time as it allows these bindings similar to Svelte and Vue and be typed. This is basically what I've been waiting for since I removed the $ prefix I had a couple years ago to do this.

I think the ability of composing arrays is interesting but realistically everything here can be done with ref today.

<button ref={combine([on('click', clickFn), somethingElse(() => state.relevant)])}>Click Me</button>

The benefit I see from the namespace syntax is no extra function calls, function wrappers, arrays etc.. it looks like a normal assignment. I was debating $ namespace.. but I something about Svelte's use seemed to flow better to me.


Modifier is interesting and I debated it briefly but ultimately landed on it not making sense for everything to be reactively wrapped. Sometimes you just want to set focus on an element etc. It's easy enough to wrap these in your custom directive since we pass the expression in wrapped.

function custom(el, valueAccessor) {
  // do I wrap it?
  const wrapped = computed(valueAccessor);
  // do I not?
  someCoolFnIDoOnMount(el, valueAccessor())
}

This decision feels more at a per binding level than a framework level. Atleast that is what my thinking is at.

Exelord commented 3 years ago

Thanks, Ryan for that explanation. I took some time and played with this syntax. If the use: namespace will get ts support then Im totally with it :) After some time, I feel very natural and comfortable with.

Although, It would be nice to have a possibility to intercept the "modifier", the same way as we can intercept the insert operation. We could eg, transform props to be always an array, or as mentioned make props reactive.

I fully agree, that not all the functions will be required to be reactive, but from my experience most of them are. Mostly cuz of props, which should react to changes. eg: on:click={this.isLoggedIn ? this.logout : this.login}.

That still can be handled higher, indeed. however, it would bring some flexibility and choice to the frameworks.

ryansolid commented 3 years ago

That's fair about event handlers. I made a decision way back not to make them dynamic to reduce reactive and DOM overhead for a case that could be handled with an inline conditional. But many libraries do offer that feature. And to be fair I probably could detect that case and support that without putting extra weight on others not using it. Just need to review things like event delegation and event binding syntax. I sort of think the pattern is awkward in that it always incurs extra cost.

Most other places I rely on the heuristic at compile time so it doesn't make as much sense to add a runtime mechanism. Unless I let the heuristic be editable. But difficult because I make a lot of assumptions around it. If I want to really make this extensible beyond my flavour of library probably worth the discussion. I've made most options so far be ones reasonable for an end user to change. Not ones that a framework would lock behind their preset. It means I've done less controversial ones thus far.

ryansolid commented 3 years ago

Now with TS support and the new types added to the declarations.

Consider this issue done.

Exelord commented 3 years ago

Yep, Thanks Ryan! :) There is still one topic with reactivity that is open but maybe I could create a separate issue for that. It's about making the expressions reactive by default, or introducing a wrapper which will allow users to handle that them selves. (unless this has been solved as well on the road)

ryansolid commented 3 years ago

I wasn't sure I completely followed. The only thing not reactive was event handlers and done so intentionally since rebinding is expensive compared to just changing the handling. Everything else follows the heuristic and is wrapped in the provided effect function. In the case of the use: directive the value accessor is always a function so it can be lazily evaluated under your own reactive context.

Exelord commented 3 years ago

on:click={isLoggedIn() ? logout : login} hmm, so in case the isLoggedIn() will change the event handler will change? or that part won't be reactive?

ryansolid commented 3 years ago

No. Event handlers is the one place purposefully not reactive. There is discussion over in Solid where I explain why: https://github.com/solidui/solid/discussions/359

Exelord commented 3 years ago

I see. It looks like though it's because of Solid as in theory this could be transpiled into sth like addEventListener(element, 'click', (e) => (isLoggedIn() ? logout : login)(e)) is that right?

ryansolid commented 3 years ago

And I've debated about that but I worry it's misleading since it always adds the "event listener" even if null and it is inconsistent. Mostly that if someone just passes an identifier instead of an expression I can't analyze I have to assume it is the event handler since signals are just functions. I can't differentiate. So it would only work with that inline syntax and not feeding another signal in. I don't like this inconsistency.

It doesn't need to be actually reactive in this case as event listeners can just read on value. I haven't considered this particularly pressing since you can wire that yourself too.

Exelord commented 3 years ago

I see your point. However this will make the learning curve here steeper, as you need to be careful what you are passing to it. Of course linters could fix that, but a field for bug will be still there.

ryansolid commented 3 years ago

The thinking is it is easier to say "on" attributes are special fields that attach an event handler function and is not dynamic. Rather than by example need to show which syntax does or doesn't work.

Although thinking more maybe this is consistently analyzable in the JSX.

<div onClick={handler} />
<div onClick={signal()} /> //wrap
<div onClick={obj.handler} /> //wrap 

<div onClick={[handler, data]} />
<div onClick={[signal(), data]} /> // ???
<div onClick={[obj.handler, data]} /> // ???
<div onClick={[signal(), data()]} /> // ???

The top examples are straightforward because we assume any dynamic expression returns a handler or array handler. Although implementation might be harder since arrays are wired up differently. The 2nd set would need different output. I will spend a bit more time to see if there isn't a way to make the implementation work consistently.

ryansolid commented 3 years ago

Ok, so there is a way of compiling the handler.

e => {
  const h = /* expr */;
  if (h) {
    Array.isArray(h) ? h[0](h[1], e) : h(e);
  }
}

This technically works but basically nullifies the benefit of event delegation since we introduce a closure per row. Now that isn't so bad except the heuristic to determine when to use this needs to assume that member expressions (object property accesses are potentially reactive). This means that onClick={props.onClick} would be de-opted. Since we are in the habit of not destructuring to maintain reactivity this would be a common case.

This is really quite unfortunate. If we had explicit syntax I'd consider it but as the common case this is not great. I could put this behind a compiler flag and have it off for Solid by default. But the alternative of the end-user doing it themselves is so easy by comparison.

Exelord commented 3 years ago

Hmm, tbh I was always against using array syntax in handlers as that can be solved with inline fn on:click={(e) => signal(e, data)}. Also, maybe I don't see something, but why someone would like to pass a signal as a handler? Do you mean memoized function?

My main point here is, if we can somehow determine inline expressions and wrap them with function to maintain "reactivity".

const aBool = createSignal(false); // that can be changed later
const noopFn = (e) => undefined
const handler = (e) => console.log(e);

<button on:click={handler} />
<button on:click={aBool ? handler : noopFn} /> // should be transpiled to (e) => (aBool ? handler : noopFn)(e)

Im fully aware this would be achievable with inline fat arrow as well but just for code consistency and clear patterns, that would make a lot of sense imo.

In other words, we would expect to always get a function, whether through an expression on directly.

ryansolid commented 3 years ago

By using the array syntax for delegated events we can remove the closure. With the function there you have 100 rows, you are making 100 closures. The array form makes zero. It's sizeable 10-15% perf improvement in large tables or nested data structures. Basically it let's us bind without the overhead of binding. https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/client.js#L69-L72. You can see how I hang it off the element itself instead of creating a new function.

I ended up adding the support for the non-delegated events as well which is basically just what you posted and has no benefit.

The problem is I know how to detect it, it isn't unlike what I'm already doing. But props.onClick could be reactive which is a common case and we instantly get a de-opt. At once point I was pushing the array syntax down to where the binding was, but there was a request to pass it through which I think is more beneficial pattern and supporting that props.onClick could be an array. And I can't reconcile it also could be reactive.

The performance consideration is definitely on my mind.

Exelord commented 3 years ago

Some time ago Ember.js was solving this issue as well. While thats true, 10k actions will be created, array reduces the DX in case of passing down actions. (you mentioned this issue higher). Bound actions was the best compromise between DX and performance.

ref: https://github.com/emberjs/rfcs/blob/master/text/0470-fn-helper.md

I was also testing this pattern while working on reactive lib, and it came out the performance is not an issue here, although in on 10k rows there was about 1-3ms slowdown, but at the end it felt unnoticeable in margin of error.

I do not want to argue about performance here, array is definitely a winner (though it still creates 10k assignments which impacts memory) but maybe the approach could be reconsidered one more time, as that would solve all reactivity issue as well as improve DX. Going all for performance may not be the best strategy.


BTW. I recently experienced performance issues with running 10k effects with ifs. I know that in solid there is a thing called createSelector, however I was wondering if we could do sth to optimize it on compilation level. 🤔

ryansolid commented 3 years ago

Yeah we are looking at this with Marko actually. Although haven't solved it for this case completely. Marko has compiled control flow, defined syntax for state, and cross template analysis, so it is much easier.

I've thought about 2 approaches:

  1. Projecting parent observability on to the model. I think though this requires 2 things I don't have here. Completely control to know that the For loop is a For loop and a knowledge of what the state is. Since more or less it is projecting the selection signal on to each data row which means we need to by looking at the template replace the data being sent in.

  2. Hoisting any comparison binding. But we'd need to understand what the comparison is. And probably still that it is a control flow. You'd replace the binding with almost a register function call to pass in variables we identify as being in scope. This isn't that unlike what I'm doing manually.

I suppose if we made it really narrow and just looked for equality comparisons we could maybe just compile in the Solid solution. I think this might be too brittle without enough ability to analyze reactive scopes. Marko accomplishes this walking in and out of child components. Might get there one day.

corbolt commented 1 year ago

Now with TS support and the new types added to the declarations.

Consider this issue done.

Hi Ryan, this topic is quite old now, but i just stumpled upon this. question: how can this be enabled, so that i won't get a TS2322 error in my UI? "use:form" in not assignable to type 'FormHTMLAttributes'. Property 'use:form' does not exist on type..

i guess there is a way to get rid of this?

thanks