WebReflection / uland

A µhtml take at neverland
ISC License
108 stars 2 forks source link

Components with more than one return path #7

Closed bcomnes closed 3 years ago

bcomnes commented 3 years ago

Hey its me again with with a question / issue.

For larger, mode'ed components that have a few 'sub-views' base on state, sometimes its clearer to have separate return paths. Below is a contrived example, but demonstrates this structure.

import { Component, useState, useEffect, render, html } from 'uland';

const MyComp = Component(function MyComp () {
  const [state, setState] = useState(false);

  function flipTrue (ev) {
    ev.preventDefault();

    setState(true);
  }

  function flipFalse (ev) {
    ev.preventDefault();

    setState(false);
  }

  console.log(state);

  if (state) {
    return html`
      <div>
        <div>hello uland (true)</div>
        <button onclick="${flipFalse}">change state</button>
      </div>
    `;
  }

  if (!state) {
    return html`
      <div>
        <div>hello uland (false)</div>
        <button onclick=${flipTrue}>change state</button>
      </div>
    `;
  }
});

render(document.querySelector('.app'), html`
  ${MyComp()}
`);

With uland, the component renders the first path, but subsequent state updates don't render the results of the other return paths (e.g. it gets stuck in !state), even though the state updates fine.

Screen Shot 2020-11-30 at 8 47 10 AM

I'm guessing this has to do with some detail around how uhtml binds template functions to the actual live dom nodes, but I can't quite figure out a solution to make this work (other than rewrite the component so there is always a single html return path, and switch / ternary inside of there).

My questions are:

Thanks! Other than this issue, uland has worked as expected and been a joy to work with. Building a web extension with it.

bcomnes commented 3 years ago

Also, I forgot to mention, an example can be found here:

https://github.com/ballpit/uland-question

WebReflection commented 3 years ago

currently, if a component returns something else not already live, there's no hot-swap replacement for that.

however, returning different things is not ideal ... think components like Custom Elements, or native nodes, a button is a button, if it becomes a div at some point, it's not a button anymore.

accordingly, beside the fact I am not sure kaboobie would work here, as there's an extra indirection ahead of time, I'd write components always within the same container.

import { Component, useState, useEffect, render, html } from 'uland';

const MyComp = Component(function MyComp () {
  const [state, setState] = useState(false);

  function flipTrue (ev) {
    ev.preventDefault();

    setState(true);
  }

  function flipFalse (ev) {
    ev.preventDefault();

    setState(false);
  }

  console.log(state);
  return html`
    <div>
      <div>hello uland (${state})</div>
      <button onclick="${state ? flipFalse : flipTrue}">change state</button>
    </div>`;
});

render(document.querySelector('.app'), html`
  ${MyComp()}
`);
WebReflection commented 3 years ago

P.S. this way you can simplify the logic too:

const MyComp = Component(function MyComp () {
  const [state, setState] = useState(false);

  function flip (ev) {
    ev.preventDefault();
    setState(!state);
  }

  console.log(state);
  return html`
    <div>
      <div>hello uland (${state})</div>
      <button onclick="${flip}">change state</button>
    </div>`;
});
bcomnes commented 3 years ago

This example is easily re-written since its contrived, I'm definitely aware of better ways to write this specific example.

The clarification I'm seeking is if its possible to have different return paths or not, and it looks like its not.

currently, if a component returns something else not already live, there's no hot-swap replacement for that.

That makes sense, though it is divergent from the high level hooks pattern in React, which does let you return different blocks of JSX from various return paths in a component.

Are you still thinking about maybe adding a hot swap feature to allow for that? If not, it would be helpful to clarify this constraint in the README as a guard rail for people trying to do it heh. I can PR something you are interested.

WebReflection commented 3 years ago

the trick to make it work could be along these lines and a PR is surely welcome ... the difficult part in here is to understand if the current component returned template is the same as the one returned before, and in such case, force an update through the render.

This might need changes in this bit too so it's either trivial or complicated, but I won't have time this week to work on that, so if you'd like to give it a shot, it'd be awesome.

WebReflection commented 3 years ago

P.S. I guess neverland has exact same issue/behavior, right? if not, we might check what's different in there, if yes, the README note should be written in both projects.

WebReflection commented 3 years ago

I think I've found the issue ... basically the top most render is not a component, hence augmentor doesn't reach it for updates, although the render is called, but there's no uland logic with plain Holes compared to Hooks ... but ...

// but this should work
render(document.querySelector('.app'), MyComp);

// and so should this
render(document.querySelector('.app'), MyComp());

in few words, the entry point for a render should be a component itself, or things at the very first level might not get updated, if changed at that level.

Your example does replace the whole node, and updates are not reflected outside components.

TL;DR this is something worth documenting but I don't have an easy fix for now

P.S. You'll read the log twice per update because of the inner + outer dance, but performance wise, the second update does nothing, so you shouldn't care (although this is something I might improve but it's not actually a bug).

WebReflection commented 3 years ago

@bcomnes at least your use case should've been fixed, but I'm not super happy about the current solution ... it doesn't look right, even if it works logically speaking.

WebReflection commented 3 years ago

@bcomnes actually I'm not happy at all with my current solution, so I am reverting. The thing is, if you pass a callback it works.

render(document.querySelector('.app'), () => html`
  ${MyComp()}
`);

I guess the reason is that each html gets its slot and if passed as Hole directly that slot never changes, while invoked at distance changes/cleanup things ... I don't know yet if there's an easy fix but I'm reverting until I find a better way to deal with holes passed along directly and components that returns different layouts.

WebReflection commented 3 years ago

@bcomnes OK, mistery solved ... when a callback is used, a new Hook is passed each time, while when the callback is not there, and it's not a component (or anything re-invokable) the Hook has been already resolved as node and passed along as is to uhtml.

There are two things to consider here:

WebReflection commented 3 years ago

So ... I think I'll keep it simple, as traversing all values and re-create each time a new Hook when any of these is a hook is a mess, but in general I can't avoid the template literal to resolve its values so this might be an update in the README, and the convention is that the render should always accept a component or a callback whenever differential returns are used, but since this always work as rule to follow, I'll make that explicit.

bcomnes commented 3 years ago

Sorry to drop off there yesterday and thanks for all the info! Absorbing asap. Will follow up if you didn't cover anything. Thanks!

WebReflection commented 3 years ago

no worries ... hooks are a headache because these work upside down (you trigger outer changes from the component inner behavior) but basically the current behavior is expected, because there's no way to avoid template literal syntax resolving the first MyComp() call, which generates a unique node that, being different each time, doesn't get a chance to be replaced, as it's already a node when the render is called, not a hook anymore.

This means I can't solve the issue internally, I need the developer to explicitly pass a callback, so that a new hook is created, accordingly with the invocation stack, each time, as opposite of receiving a new node/hole that represent a single entity and can't be replaced (because a component can return multiple elements and be a fragment instead, but the DOM logic is responsibility of udomdiff, not uland, so changing nodes at runtime is a road to hell for updates in the wild).

Not sure this explains much, as most details are internals, but basically the fix is: use an arrow function instead of passing an html result right away.

bcomnes commented 3 years ago

sounds easy enough!

Hooks are a weird beast, they have that strange no-conditional logic at the top of your component around the hooks requirement too. So something as simple as a requirement to using an arrow function in your render mount really isn't that much to ask.

WebReflection commented 3 years ago

yeah, I've tried to explain the whole stack dance in this post and yet it's a mental model, as hooks library author, that's not always straight forward to grasp ... here, as example, it took me way longer than it should have, to understand why things were going one way, instead of another.

hooks are great for DX, but horrible in general in practice, technically speaking ... it's a convoluted inside-out dance that is everything but easy to debug too. Luckily augmentor never had too many issues to date, but here there's something I can't really solve without a callback, as template literals tags can't be controlled when values are resolved, otherwise there would be a way to fix this in an even better DX experience.

WebReflection commented 3 years ago

@bcomnes FYI not only 0.4 is smaller and faster, but you won't see anymore more than an invoke per component update, as the whole rendering is bound to updates now, thanks to core changes based on new libraries that do a much better job in terms of orchestrating updates.

In theory nothing changed for you, but I recommend to try 0.4 out whenever you can 👋

bcomnes commented 3 years ago

Amazing! Will update in my project and let you know the outcome. Thank you!

WebReflection commented 3 years ago

also forgot to mention there's no need for a callback anymore as render second parameter 🎉

WebReflection commented 3 years ago

demo here: https://webreflection.github.io/uland/test/multi-return.html test file here: https://github.com/WebReflection/uland/blob/master/test/multi-return.html

bcomnes commented 3 years ago

Just following up here, trying out 0.4 and its working great! With our without a callback, not that I have a preference either way, other than its nice that there is one less footgun.

bcomnes commented 3 years ago

And I just extracted some shared state/effect code from a component into its own function, and successfully re-used it across two components. Worked great! I was thinking maybe I had to wrap the custom hook in some kind of wrapper, but nope! Just useState/useEffect in a function, and call the that custom hook in a component and it works. Bravo!