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

Best way to use a ternary which doesn't re-render each time? #115

Closed
nettybun opened this issue Jun 10, 2020 · 10 comments
Closed

Best way to use a ternary which doesn't re-render each time? #115

nettybun opened this issue Jun 10, 2020 · 10 comments
Labels
good first issue Good for newcomers question Further information is requested

Comments

@nettybun
Copy link
Contributor

Hey I noticed that using a function+ternary to switch-render content will (maybe obviously) re-render content each time:

const messages = observable([])
const Page = () =>
  <main class="bg-purple-100 antialiased justify-center p-8">
    {() => messages().length < 5
      ? <MyComp/>
      : <em>Gone</em>
    }
  </main>

Also factoring out the component into to prevent building MyComp from scratch like this...

const renderedMyComp = <MyComp/>
const messages = observable([])
const Page = () =>
  <main class="bg-purple-100 antialiased justify-center p-8">
    {() => messages().length < 5
      ? renderedMyComp
      : <em>Gone</em>
    }
  </main>

...is better but still removes and adds the node to the DOM each time messages changes. If the component has side effects this can be undesirable. I wrote a workaround but I was curious if you've come across a different/better way since you have most experience with this and it's a pretty common operation:

const renderSwapA = <MountTest/>;
const renderSwapB = <em>Gone</em>;
const renderSwapMarker = document.createTextNode('');

const messages = observable([])
const Page = () =>
  <main class="bg-purple-100 antialiased justify-center p-8">
    {renderSwapMarker}
  </main>

subscribe(() => {
  const parent = renderSwapMarker.parentElement;
  if (!parent) throw 'No parent for renderSwapMarker';

  if (messages().length < 5) {
    if (renderSwapB.isConnected)
      api.rm(parent, renderSwapB, renderSwapMarker);
    if (!renderSwapA.isConnected)
      api.add(parent, renderSwapA, renderSwapMarker);
  } else {
    if (renderSwapA.isConnected)
      api.rm(parent, renderSwapA, renderSwapMarker);
    if (!renderSwapB.isConnected)
      api.add(parent, renderSwapB, renderSwapMarker);
  }
});

It's a lot of code (but could be reused as a function I'm sure there's ways to make it easier to invoke). Is this what you've been doing as well?

Thanks!

@ryansolid
Copy link

ryansolid commented Jun 14, 2020

You are correct. You need to separately wrap the condition if you don't want it to execute over and over. Sinuous reactive system I believe does not support notify on change in its computeds so writing to a observable is the best approach. The most basic form would be an if conditional. Handled completely from the reactive point of view ignoring rendering it could look like this:

function iff<T>(cond: () => boolean, handler: () => T): T | undefined {
  const o = observable();
  let prev;
  computed(() => {
    const next = !!cond();
    if (prev !== next) {
      prev = next;
      o(next);
    }
  });
  return computed(() => o() && handler());
}

This should automatically handle disposal on dead branches etc. You could have 2 callbacks I suppose to handle true and false cases. Or call handler with the boolean value, although that feels more odd since you end writing the ternary in the callback handler anyway.

I should point out another consideration is sampling the handler in the returned computed. I tend to do that, but it has tradeoffs. The benefit of sampling is some random thing doesn't cause the parent computed to re-execute. But then again there are cases where maybe someone wants that.

EDIT: For a general solution I use an approach like this as hoisting (ie creating the component outside of the condition and choosing to insert or not) has some side effects. Firstly disposal always belongs to the parent. That's probably fine here but it's worth mentioning that if you have 2 branches you are actually keeping them both around. Second if the offscreen branch is reactive and ties into things like global store it's actually updating the in the background potentially. This is kind of cool but it can be this tax you are paying. Which bringsme to the last point. Often hoisting is not an option. If something depends on something else being there you can't just create it ahead of time. It's super common in view logic to say not show a panel before the data is loaded. You can can setup the reactive tracking without having the data but keep in mind all downstream code will have to treat that data as if it is nullable.

Personally this means I'm not particularly interested in storing the branched nodes in most cases. But people have definitely been interested it leading to solutions that involve caching the components at first render under this pattern. I played with a Component singleton helper (HOC) that basically you make a Wrapped Component instance per unique instance and it always runs the first time but then uses that component if it is ever executed again. I think this is a pretty broken pattern though but it is doable. I personal feeling is the hoisting is independent of this and you can use a conditional like I posted, and then choose where the creation of the child component happens. Wrapped in it or above the statement in the parent context.

@nettybun
Copy link
Contributor Author

nettybun commented Jun 14, 2020

Thanks for good points! I hadn't considered disposal of branches as much as I should have. Right now I'm storing them in the global scope and spending time rendering both branches before ever even knowing if I'll render them at all (if <Page/> isn't called for instance).

You're right that it can be difficult to pre-render components before data/context is available and then need to account for the case of data being nullable. I've dodged it a bit by using mount hooks to only do data-related work when connected to the DOM, but that doesn't solve the problem of preventing extra work entirely, plus it adds layers/complexity.

I'll look into the iff() function and see about adapting something similar that doesn't leave conditional children never being disposed.

@luwes luwes added good first issue Good for newcomers question Further information is requested labels Jun 20, 2020
@luwes
Copy link
Owner

luwes commented Jun 20, 2020

@heyheyhello is this an option? #6 (comment)

uses https://github.com/luwes/memo

ah yes, if messages() changes it will trigger a re-render each time.
@ryansolid's solution should fix that.


I recently added a test that mimics Surplus's S.value.

function value(current, eq) {
const v = o(current);
return function(update) {
if (!arguments.length) return v();
if (!(eq ? eq(update, current) : update === current)) {
current = v(update);
}
return update;
};
}

That could help too in this situation I think but you would have to keep one more observable up to date which would track the boolean value of messages().length < 5.

The end goal is to have an extra observable that triggers a computation only when the condition changes, not some value in the condition.

@nettybun
Copy link
Contributor Author

Hey @luwes good to see you! Thanks for the ping about memo, I checked out the codesandbox but noticed that if the condition is changed to

html`${when(
  () => count() %2 === 0,
  cond =>
    cond
      ? ...
      : ...
)}`

Where the ternary is flipping back and forth, it re-renders the count() component each time. Meanwhile in the render-swap version I posted the MountTest component would be added or removed from DOM but never re-rendered (re-built). I imagine for routers or some swappable components on a page it would be good to have some reassurance that the component won't rebuild.

Memo is super cool but to me it's a bit of a black box - even when reading the source code I'm hesitant to dive into it because it suddenly adds a layer of mystery to where content lives and will be re-rendered. Locality is rad, and I want that, but I'll probably poke at more of a swap-based version instead of serializing function arguments and such. However I do recognize that I'm the type to want to understand everything I'm using 😅 and memo might be perfect for a lot of people's use cases

@luwes
Copy link
Owner

luwes commented Jun 20, 2020

the important thing to check for is the console logs I think.

it's normal the b tag gets updated because there is the variable count that gets incremented each time.

normally that b tag should be the same one I think.

memo is pretty much the same as a memoize function. memoizes the result of the function based on the arguments

@luwes
Copy link
Owner

luwes commented Jun 20, 2020

oh ok I see what you mean, each branch has its own b tag. those are not the same.

@nettybun
Copy link
Contributor Author

nettybun commented Jun 20, 2020

No no you're right... that's so weird I swear I was looking at the console and seeing "render count" every %2 cycle. But I just checked again and now it's working (?!) So, that's awesome.

@nettybun
Copy link
Contributor Author

Hey I made a thing!

I think memo definitely has a time and a place but honestly the idea of JSON.stringifying arbitrary function arguments will keep me up at night, so, here's a when() that doesn't use memo and is still really short~. (If you don't read TS just see the codesandbox link; it's a fork of @luwes's when())

const when = (
  condition: () => string,
  views: { [k in string]?: () => Element}
) => {
  const rendered: { [k in string]?: Element } = {};
  return () => {
    const cond = condition();
    if (!rendered[cond] && views[cond])
      rendered[cond] = root(() => (views[cond] as () => Element)());
    return rendered[cond];
  };
};

This has the added benefit of any number of switch cases:

    <div>{
      when(() => count() === 0 ? '0' : count() < 10 ? '0..10' : '>= 10', {
        '0':
          () => <p>There's no messages right now</p>,
        '0..10':
          () => <p>There's {count} messages; which is less than ten...</p>,
        '>= 10':
          () => <p><em>So many!</em> There's {count} messages!</p>,
      })
    }
    </div>

Components are only rendered as needed.

Here's a sandbox: https://codesandbox.io/s/cool-swirles-gqudr

image

My original OP can be replaced now with

const Page = () =>
  <main class="bg-purple-100 antialiased justify-center p-8">
    {when(() => messages().length < 5 ? 'T' : 'F', {
      T: () => <MyComp/>
      F: () => <em>Gone</em>
    })}
  </main>

Which is the same lines of code and no magic ✨✨

@ryansolid
Copy link

@heyheyhello Looks good. Combination of approaches using the hoisting with root management to handle isolation and disposal. I guess the interesting part is nothing is released until it is unmounted which is probably fine since this isn't for rows in large lists and probably for like tabs or something. I think given the goal this is about the most elegant approach and how I would have done it this way myself. (I actually use a similar approach for one of my prototypes for Suspense where I have multiple futures living at the same time while time is suspended before collapsing back down to the simple case for normal operation).

There are couple gotchas with this goal in general but I think it comes with the territory. Be careful with state updates updating offscreen nodes in ways that would break them. Like setting values to undefined that are read from etc.. It's mostly likely fine but the consumer needs to understand that even though the nodes are gone from the view they aren't gone.

@nettybun
Copy link
Contributor Author

Thanks for the feedback. Yes my use case is for tabs or swapping small snippets, but great points about the potential gotchas.

Maybe there's a way to write it that does a delete rendered[cond] to free up unmounted elements. Thankfully it's a simple enough function for someone to understand and choose to hack it for their use case.

I suppose for state updates I'd have to write some code with onAttach/onDetach hooks to tell the component to ignore state changes (somehow?) when it's disconnected from the DOM...not sure. I'll cross it when I get there haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants