luwes / sinuous

🧬 Light, fast, reactive UI library
https://sinuous.netlify.app
1.05k stars 34 forks source link

Add Hacker News clone? #89

Closed mindplay-dk closed 3 years ago

mindplay-dk commented 4 years ago

So I think I'm done with my Hacker News clone for now:

https://codesandbox.io/s/sinuous-hacker-news-dqtf7

I would have liked to add some progressive loading - it's pretty aggressive on the HTTP requests now, since every comment in a thread requires recursive HTTP requests for the individual items... probably a bit bad on items with too many comments.

Let me know what you think? 🙂

mindplay-dk commented 4 years ago

Uhm, in case you already looked - no idea what happened, but CS had not been saving my changes to one of the files, apparently it was just pretending. 😐

Fortunately, there was almost nothing in that file, it was easy to recreate.

Should be working now. 😄

mindplay-dk commented 4 years ago

There is one thing I'm not happy with, and I'm not sure if there's a better way to do this.

If you look at reveal.js, this is how I create the animation when CommentItem instances get recursively loaded and rendered in comments.js.

If I was working with plain DOM, what I'd use is something like this:

function appendTo(parent, child) {
  parent.appendChild(child);

  const height = child.offsetHeight;

  child.style.height = "0";
  child.style.transition = "0.5s";
  child.style.overflow = "hidden";

  requestAnimationFrame(() => setTimeout(() => child.style.height = `${height}px`));
}

That last line waits for the next animation frame, then waits for CSS to initialize the start of the transition before setting the height - I picked that up from this answer.

Unlike that function, the reveal function in this demo needs an extra requestAnimationFrame before measuring the offsetHeight - because you can't measure the height of an element until it's parented. It doesn't have to be rendered (and it isn't, since I'm immediately changing it's height to zero after measuring it) but it does have to be parented for the DOM to calculate the actual height based on the layout parent.

I asked previously about effects, and your answer was about cleanup, which for some reason I couldn't get to work here.

The component doesn't know when it gets mounted, and I tried cleanup, but it didn't seem to work for this case - offsetHeight was zero, so cleanup seems to happen before the component has been mounted. (?)

So the extra requestAnimationFrame is a work-around because, unlike appendTo, I'm not in charge of parenting the new element, and it's just sort of a "fingers crossed" approach, where I'm hoping Sinuous will have parented the element by the next animation frame so I can measure the size.

But that code is non-obvious and I'd prefer something more explicit that actually explains when/how it's doing this - it feels really brittle, and seems to rely on an implementation detail in Sinuous.

Is there a better (safer, more explicit) way to approach this?

luwes commented 4 years ago

waaw this is really cool! code wise it looks great to me.

one request for each comment is indeed quite taxing, if there is a comments collection endpoint that would be better.

not sure if it's intentional but since the stories endpoints are cached the generated DOM creation of the stories per type could also benefit from being cached. right now every top menu link click will re-render the whole list.

checking your last comment now

luwes commented 4 years ago

yes, cleanup is run when the computation re-runs. it runs on removal but also just when the computation is re-run. it can't work as a mount hook and there's not a super easy way to do it unfortunately.

related issue https://github.com/luwes/sinuous/issues/73 worth checking @ryansolid's comments and solution too https://github.com/ryansolid/solid/issues/36#issuecomment-500546082

a few days I created this lib https://github.com/luwes/disco just for having explicit connected and disconnected events for DOM elements. at the moment I think that's the best way to go.

observe() takes string selectors too so something like the below would add a connected event to the comment items when it attaches to the DOM.

import { observe } from '@luwes/disco'; // waiting on the `disco` npm name 😄 

observe('.comments-item');
luwes commented 4 years ago

seems to work well with connected events https://codesandbox.io/s/sinuous-hacker-news-yx36w

ryansolid commented 4 years ago

So the extra requestAnimationFrame is a work-around because, unlike appendTo, I'm not in charge of parenting the new element, and it's just sort of a "fingers crossed" approach, where I'm hoping Sinuous will have parented the element by the next animation frame so I can measure the size.

I get the reaction, I've just done precisely this.. well setTimeout 0, so many times in the last 10 years of using KnockoutJS I stopped even thinking about it as a hack. I'm old school though, we did this with jQuery before that too. Because cleanups occur you can ensure you can cancel the timeout in the weird chance it is removed before you get the chance. The only reason I've come across to use mutation observers is if you managing DOM elements independently of the render lifecycle. The scenario in that issue was holding on to the Tabs that were disconnected when not selected and re-inserting them. In that case you care about actual connectivity. But for DOM measurements etc.. these libraries render synchronously. So even by the next microtask you are probably good.

PS. Awesome demo.

mindplay-dk commented 4 years ago

seems to work well with connected events

tbh, I'm not very fond of this approach either - reading the code still doesn't make it obvious what observe('.comments-item') does or why onconnected works.

I understand how it works, but I'd be concerned about using something like this at scale or in a team - like when you come back to an old jQuery project and have no idea where listeners get attached or why dispatching a click-event somewhere does anything at all.

Any chance for some kind of native events in the framework?

I'm thinking back to an older version of this package, which had these incredibly useful element-level life-cycle events, which enabled things like animations on creation/removal - for example:

      <div class="notifications__item notifications__item--hidden"
      key={ item.key }
      oncreate={ el => setTimeout(() => el.classList.remove('notifications__item--hidden')) }
      onremove={ (el, done) => {
        el.classList.add('notifications__item--hidden');
        setTimeout(done, 1000);
      } }>

onremove here was a personal favorite - being able to defer the removal of an element was incredibly useful for things like deleting items from lists.

Not saying element-level life-cycle events are necessarily the right approach - this is just an example of something that was a good fit in a different framework.

I mean, surely the framework knows when it's adding/removing elements? Without having to ask the DOM to observe and notify it of changes that it itself requested? 🙂

Disco looks pretty cool and I could think of some good uses - I've actually often wished for something like this, and I've seen some pretty horrible solutions in the wild where something similar was achieved by polling the DOM and repeating queries. 😬

But in the context of Sinuous, going around the framework back to the DOM, to see what the framework is doing... it's... not elegant? I'd prefer the framework itself provided an answer.

Tall order? 🙂

luwes commented 4 years ago

that's the thing, the framework doesn't know when a created element is attached to the DOM. 😃 it's because it creates inside out, being based on the hyperscript like calls.

h('div', 'text child', h('div', 'div child'));

you can see here, the right most div is created before its parent and the parent might not be attached to the DOM yet.

that's one advantage of VDOM. just thought of re-dom, how do they do it? hmm

ryansolid commented 4 years ago

I think it is just what they do. That is the library(ReDOM) essentially. Their whole system is managing this connected state. It is always a bit of a challenge for single pass libraries not based on DOM lifecycles (basically every non-VDOM library that doesn't use Web Components) but especially challenging for reactive libraries that have no centralized render cycle.

It is possible to do here but it requires a whole another layer of book-keeping. If you collected the nodes that would be connected for the first time on each operations, and would be removed you could iterate over the list at the end of each update. You basically need to add ReDOM on top of everything we are already doing and then add the overhead to scheduling of reactive updates.

I think the only solution is based on the type of work queue a job and maybe attach information on the DOM nodes themselves. I looked into this and ultimately decided it wasn't worth it and if someone really cared a mutation observer approach made sense since it is so easy to add. Like disco or even closer to disconnected where they masquerade as DOM events. Even adding that sort of functionality to the library would be simpler than actually trying to track additions/removals like ReDOM.

I know this was already linked but like it's so simple. https://codesandbox.io/s/dom-lifecycle-u0gbd. Is library agnostic and does the trick and the consumer is no wiser. Because ultimately even though it's a whole other 2nd pass any approach we took would be too.

luwes commented 4 years ago

good you posted your example again, I read it wrong before. thought it had some performance drawbacks but it's probably the most performant way to do it. checking if the dis-connected methods exist on the node.

ryansolid commented 4 years ago

Yeah I mean it does have the overhead of walking that portion of the tree each time something is added or removed. But I mean it comes with the territory if you want this behavior. It's a single mutation observer so it's not bad for performance. I'm not going to add it to a benchmark for sure but it's reasonable.

mindplay-dk commented 4 years ago

that's the thing, the framework doesn't know when a created element is attached to the DOM. 😃 it's because it creates inside out, being based on the hyperscript like calls.

Hmm, yeah, the library is not responsible for adding the root node of an app to the document - but it is responsible for adding all the nodes to their parent nodes, right?

It's also responsible for inserting nodes into the existing structure during updates.

So the only thing you don't have a hook for, is when the structure you've created gets added to the document - you'd have hooks for every other insert/update/delete operation.

So it seems maybe the problem is how apps start up:

document.body.appendChild(App());

There's nothing that lets you know the app root has been mounted.

Can't we just add a simple hook for that?

mount(App(), document.body);

And I guess maybe that hook just broadcasts a "connected" message to all the children, similar to what you're doing with disco.

Most other frameworks have an initialization hook, so I don't think a requirement like that would be too surprising to anyone?

@ryansolid to your last comment, I'm not really too concerned about the performance - though MutationObserver does need a much slower polyfill in IE, but it's more the principle of using an external facility to solve an internal architectural problem that bothers me.

Even if it solves the problem, the framework shouldn't need a facility to tell the framework what the framework itself just did. Should it? It's quite possible I'm missing something here. 🙂

ryansolid commented 4 years ago

Hmm.. I suppose if you are going to handle it from a DOM node perspective there are options. I was mostly considering this from a Component perspective since that's what VDOM libraries do and where you get a concrete framework like benefit.. Like a hook. It is more consistent than adding arbitrary handlers on a per element basis. Nothing else in the library does that. And that's a much tougher problem since Components are ephemeral. They are just function calls.

It should be possible to check the connected state of the parent node (isConnected, polyfillable by Mutation Observer in IE11) at any insert point and then basically run the same code as the Mutation Observer to check down the tree. There may be performance cost/complexity especially on removal as shortcuts couldn't be taken. Although it sort of the same thing as the mutation observer solution and if built that way that behavior wouldn't be able to opt-in/opt-out of that behavior as easy. I suppose some sort of global config. But to an end user(who didn't need to worry about polyfills) both solutions would be indistinguishable. You include the package or you don't (via import, config etc).

Basically from the libraries perspective DOM updates are side effects. It'a a bit weird to think of it that way. But there is no data structure it's managing that is DOM specific, no VDOM, no linked DOM tree templates, no Component hierarchy. Just independently executable reactive nodes. We just happen to put some DOM insert and cleanup code in them for rendering. No cell is really aware of the whole system. All context is sort of injected orthogonally. Which means the skies the limit on the powerful things we can model with these simple primitives, but by default they don't know what they are doing. I've modelled Suspense and other VDOM mechanisms without touching the rendering at all. It's all independent. Someone in the Community could have done it and released their own package without even changing the library. Which is very different than say Preact's migration to Preact X.

So there are a number of options from simplest to most complicated.

  1. Consider this a data propagation timing issue due to a side effect and set a timeout.
  2. Add a mutation observer to the root and run DOM node specific methods on addition/removal.
  3. Add connected checks to renderer to inserts/removals that do some DOM node specific post processing.
  4. Add connected checks to renderer to inserts/removals + mark nodes for Components with component id (including handling fragments), so that during these operations we can use a context lookup to run mounted/unmounted hooks for each Component rather than handling it on a per DOM node basis.

There are probably a few others. Definitely interesting areas to try. Personally in the past decade of using KnockoutJS I never really moved past 1.

EDIT: @luwes I apologize I kind of got carried away as usual. I think there are some options here. I'm not going to push my opinion too hard here as Sinuous is a completely independent project than my perspective of the overall approach.

luwes commented 4 years ago

all good @ryansolid, it's very useful information.

@mindplay-dk I'll look into this further, maybe redom has a simple solution for it however to add it to the Sinuous core will probably add too many kB's. I'd like to keep the hello world example below 1.5kB.

Even if it solves the problem, the framework shouldn't need a facility to tell the framework what the framework itself just did. Should it?

one one side I agree but on the other I also think the DOM api's got the element appended and then being able to solve the connected and disconnected hooks/events with other native api's is not a bad way to do it.

the observe() function from disco does look somewhat out of context, maybe this can be improved with more explicit api's.

mindplay-dk commented 4 years ago

Maybe I didn't explain that idea well. 😊

When I say "hook", I'm not talking about React-like hooks or something like that - I'm using the word in the most general sense, meaning basically "something that gets called when something happens".

The framework has add and removeNodes functions internally, so that provides two hooks - the framework knows when nodes get added and removed. (via the frawework, obviously - that's all I'd expect; otherwise, something like mutation observer would be the only way to know that.)

What the framework doesn't know, is when do these nodes get added to the actual document - because that's not something the framework does right now, the userland code does that manually.

All I'm suggesting a mount function would do, is add a given node to a given parent, and (assuming this is the prescribed, correct way to mount a new root component) this provides a hook - now the framework knows when this happens.

So after add or mount, we now have a hook for calling a function that notifies any event-listeners of any children - naively, something like:

function notifyConnectedChildren(parent) {
  parent.querySelectorAll("*").forEach(node => {
    node.dispatchEvent(new Event("connected"));
  });
}

I'm ignoring some details like not broadcasting this event more than once - and this is a totally naive approach that won't scale or perform well at all, but that would work, right?

I don't think this adds anything substantial to the footprint.

Usage would be similar to Disco, you just add an event-listener.

Alternatively, maybe the framework provides an optional submodule with a helper of some sort? I tried something here, which is similar to what I was doing anyhow - but if it's part of the framework, that seems less like a hack, since that means the package promises that the behavior of this function will correlate with the implementation details of the framework. It doesn't do much, it mostly prescribes a way to solve the problem, and removes any worries about relying on implementation details.

That approach probably only works for connecting and not for disconnecting though.

luwes commented 4 years ago

Makes sense, the hook would be more like a mount() or render() function in other libs. This would almost be a job of a consumer of Sinuous I think but it could be another package. I'm happy to put in any hooks that would be required to make this work. The internal api can already be monkey patched by other packages, api.insert, api.property, api.add, api.h, etc. sinuous-context is a good example https://github.com/theSherwood/sinuous-context/blob/master/src/context.js#L53

samadadi commented 4 years ago

I added onadd hook using monkey patching to this project and used it instead of reveal function in this forked link: https://codesandbox.io/s/sinuous-hacker-news-8k6hq?file=/src/app/comments.js. But I don't know the way onremove hook should work. should onremove be called before sinuous removes the element from dom or after the element removed?

ryansolid commented 4 years ago

The only real benefit I can think of before remove is if it could actually prevent and schedule removal. It's hard to do that in a generic way without all helpers/reconciling being aware of this intermediate state. However I'm pretty sure that is conceptually what people would be looking for.

mindplay-dk commented 4 years ago

What I'd really like to use this for, is just to defer removals - for example, when removing an element from a list after a delete operation, if it were possible to just leave the DOM element for a moment while it fades out or collapses, this really helps with user perception by avoiding any visual stutter.

Yeah, this probably affects list reconciliation, which definitely seems like a non-trivial problem. 🤔

As mentioned, I've only ever seen one library successfully pull this off...

nettybun commented 4 years ago

I commented a potential concept here https://github.com/luwes/sinuous/pull/117#issuecomment-656236179 but let's talk in this thread since the closed PR might be harder for people to find.

which definitely seems like a non-trivial problem. :thinking:

Agreed... The codesandbox I linked (in the PR thread) shows that reparenting a node will cause Sinuous api.rm to skip it - which is neat - so there might be a way to hot-replace a list item with a new parent wrapper. I haven't tried it yet, and am already having weird list behaviour in the sandbox I linked as is. Lets just say the GIF I recorded in my other comment was the "happy path".

nettybun commented 4 years ago

It's @luwes' magic line here that lets the skip trick work (source):

export const removeNodes = (parent, startNode, endMark) => {
  while (startNode && startNode !== endMark) {
    const n = startNode.nextSibling;
    // Is needed in case the child was pulled out the parent before clearing.
    if (parent === startNode.parentNode) { // 👈👈👈
      parent.removeChild(startNode);
    }
    startNode = n;
  }
};

And that I call onDetach before letting Sinuous' api.rm run (source):

const rm: typeof apiRef.rm = (parent, start, end) => {
  // Parent registered in the tree is possibly different than the DOM parent
  const treeParent = searchForAdoptiveParent(start);
  const children = tree.get(treeParent);
  if (children)
    for (let c: Node | null = start; c && c !== end; c = c.nextSibling) {
      children.delete(c);
      tracers.onDetach(treeParent, c); // 👈👈👈 First
    }
  return apiRef.rm(parent, start, end); // 👈👈👈 Second
};
nettybun commented 4 years ago

Got it. @mindplay-dk Does this work?

https://codesandbox.io/s/sinuous-remove-transition-3fx44

Peek 2020-07-09 22-38

Marked my own other comments as outdated because this just works so so much better than whatever I was doing before

mindplay-dk commented 4 years ago

@heyheyhello very cool! 😃 ... This would work for something like a notification list, since it's using native Array.map - the typical use-case for me would be something like a tabular form, so something involving the Sinuous map API would be required, I think? Since otherwise we'd lose input state when doing updates, right?

nettybun commented 4 years ago

Well it's just replacing the api.rm function so it should work with anything being removed, not just a list. I used Array.map purposely because it removes and recreates each time. If I used sinuous/map for an append only list there would be no removals 😛

However. I think sinuous/map uses native DOM APIs like parent.removeChild instead of api.rm... I haven't tried yet but I remember map not working for sinuous-lifecycle for that reason.

We might have to patch map (maybe entirely) to get the special remove function both of our use cases require

mindplay-dk commented 4 years ago

I see, yes. Hmm. What do you think about the React-style "transition group" approach? It's essentially diffing internally, which isn't a really attractive prospect in the context of Sinuous - I don't think we want to duplicate that logic. But maybe something similar could be achieved with a component of some sort, which internally uses sinuous/map and places every child in a wrapper component to manage the animation or something..?

nettybun commented 4 years ago

It's essentially diffing internally, which isn't a really attractive prospect in the context of Sinuous

I can only speak for myself but if I liked diffing and vdom I'd be using React 😏

Sinuous is nice because it doesn't do that, but I still see what you mean. I'll try and think about how I'd design a tabular data form for input state and rows/cols that fade out. Do you have a more specific example of what you're building? Because the solution might not be to patch the core framework, it might be best implemented as logic at the component level.

mindplay-dk commented 4 years ago

@heyheyhello a minimal example could be just a list of names - so each row would be just an input and a remove-button. If that works, it should work for rows with multiple/different types of inputs too, right?

mindplay-dk commented 4 years ago

(by the way, another thing that's nice about Sinuous is, if you do have to diff with sinuous/map, it's shallow, not deep like it is in React - you're just adding/removing items from the list, not drilling into all the children of each item. Man, this library makes so much sense!)

nettybun commented 4 years ago

Right but that doesn't inheritly need any diffing. It's just a list so you can use sinuous/map to render the items, and if you wanted to delay removal of items just set the remove button event listener to something like:

onClick={() => {
  element.className += "fade500"
  setTimeout(() => nameListObservable(nameListObservable().splice(...)), 500)
}}

(I'm typing on a phone so gimme some slack) but you'd basically not involve map in the pre removal phase at all... Just invoke map by changing the observable after you've done all your pre remove stuff

ryansolid commented 4 years ago

Yes.. people are strangely adverse to using set timeout. Claiming it is hacky. It isn't limited to conversation earlier in this thread. It comes up every time.

Something like transition group isn't too bad. It too can be shallow comparison. It's just a transforming buffer. It remembers its previous state and marks/collects nodes that are new/old and returns the hybrid state. This way in something like the example above only the new row would fade in not the whole thing. Or only the single row would fade out.

If it were just limited to top level nodes it would probably be not too difficult to implement. The challenge is making sure multiple actions happening before the previous ended would properly work and not stomp on them.

The easiest way is to slide itself between the reactive mapping(data to DOM) and the DOM updates(old DOM to new DOM). I've seen map functions offer per row transformations but that might complicate the core reconciler to handle the case. If one could simply handle the intermediate list holistically before insert only the places that needed to pay the cost would.

Anyway this topic has been on my mind lately. There might be even better approaches. Svelte's animation is impressive and I suspect syntax aside something similar can be done in this area.

mindplay-dk commented 4 years ago

Not averse to timeouts at all. 😉

But delaying the removal itself can be problematic - you're delaying the update of the model, which means your model could be temporarily in an inconsistent state. Or you'd have to add "removing" properties to your items, which means you're putting UI details in your model and dealing with them explicitly.

For example, say you're removing an item from a shopping cart. If I remove an time, I'd want the total to update immediately - not half a second later when the animation ends. You can accomplish this with a computed that filters items that are being removed, but it feels inelegant. I'd prefer having the animation details encapsulated in a component somehow, so I can update the model the moment the update is supposed to happen, so the animation becomes just a side-effect.

Anyway this topic has been on my mind lately. There might be even better approaches. Svelte's animation is impressive and I suspect syntax aside something similar can be done in this area.

I took a look at Svelte's animation demo and frankly I'm not that impressed. The animation itself looks good, but things are still popping/jumping when you add/remove something. The technique they applied looks solid though. There's an older library implementing this pattern, might be useful for reference.

I wonder if it's possible to combine the FLIP technique with a wrapper or placeholder element and animate the width/height of that, so that surrounding items would correctly transition into place while the item's occupied space shrinks or expands... 🤔

luwes commented 4 years ago

Closing here. Thanks for the great conversations! ❤️

mindplay-dk commented 3 years ago

I thought of a simple idea to solve this, maybe.

I would just add something to the API, like:

function mount(node) {
  if (node.ownerDocument.contains(node)) {
    node.dispatchEvent("mount", new Event(mount));
  }
  return node;
}

And then, the procedure for mounting something in the DOM would be e.g.:

mount(document.appendChild(<App/>));

The library internally would call mount anytime it adds a child - if the child's parent has not been added to the document yet, nothing happens, but when the root of an update eventually calls mount during some larger update, it will have a parent, and all the children will get notified via a standard DOM event that they can listen for.

Granted, this doesn't work for components that don't render any elements - but, typically, the reason you're interested in knowing when elements get mounted, is because you need to measure their size, or something layout-dependent, and this should work well enough?

Not sure if an unmount function (and maybe deferred removals) could be implemented using a similar strategy.

nettybun commented 3 years ago

@mindplay-dk This is what my sinuous-lifecycle package does, specifically tracking when children nodes join parents to form a larger tree without yet being mounted to the page, and then when it is mounted all children are notified. I knew Node#contains would be a simpler solution but was concerned about how much work that entails under the hood:

In both Chrome and Firefox there's the full walk of the ancestors of a node and the consideration of whether they're in the right document, shadow root, etc. I can't tell how expensive it is. It might be a lot faster than JS walking, but I opted to assume the worse when writing sinuous-trace, so I build a tree of components (and guards email me if you wanna chat), under the idea that you'll be able to skip a lot of nodes by hopping...

Kinda like https://en.wikipedia.org/wiki/Saltatory_conduction haha ⚡

No idea if I'm right. There's a chance the C++ implementations are so fast that any WeakMap hopping in JS doesn't even compare.

mindplay-dk commented 3 years ago

@heyheyhello there's also isConnected in modern browsers, that might be faster? this other approach could be used as a very lightweight ponyfill for legacy browsers. (either way, I'm more concerned about ergonomics than about winning benchmarks.)

nettybun commented 3 years ago

Might be. Chrome uses a flag it seems for isConnected. Its own bookmarking system for nodes. Looks even O(1) at first glance

mindplay-dk commented 3 years ago

Hm, events of course were designed for things like UI events - clicks, etc... so they bubble from their target towards the document root - which is the opposite of what I wanted, to broadcast an event to all of the child-nodes. That's not a feature of the DOM event system, it seems. Events go up, not down. (well, they go down during capture, but only along the parent path to the root.)

I mean, you could querySelectorAll of course, and manually distribute the event that way - but at that point, there's no real advantage to the DOM event mechanism, which would propagate all those events upwards along the path to the root. I can't think of any reason that would be useful. (Perhaps this is some of the same thinking that lead to the deprecation of mutation events.)

DOM events don't seem like the way to go.

Hmm, I really want to think of a simple solution to this. 🤔

How about collecting all newly created nodes in a global array? Whenever I say "global", I feel dirty, but... We could simply collect all the nodes pending that "mount" notification, and then flush that list and broadcast notifications after connecting a node to the document? Would that be... bad?

mindplay-dk commented 3 years ago

I implemented this in my toy sinuous clone - it seems to work well enough, and it's simpler than I thought it would be:

const isConnected = 'isConnected' in Node.prototype
  ? node => node.isConnected                   // modern browsers
  : node => node.ownerDocument.contains(node); // IE11 and older browsers

let onConnectListeners = [];

export function onConnect(f) {
  onConnectListeners.push(f);
}

export function connect(node) {
  if (isConnected(node)) {
    onConnectListeners.forEach(f => f());

    onConnectListeners = [];
  }

  return node;
}

isConnected is my "ponyfill" for Node.prototype.isConnected.

Components register for notification with onConnected, and connect checks if a given node is connected, and if so, simply flushes all the listeners - this works, because the only time anything is going to get connected, is when the root node gets connected; or in other words, if the root node gets connected, that means everything that has been created has also been connected. (Assuming no one adds onConnected listeners without actually adding the created nodes - which, why would they.)

When you bootstrap your app, you have to call connect, e.g.:

connect(document.getElementById("app").appendChild(<MyApp/>));

Internally, I call connect after replacing a reactive node - and in map after diffing. I don't know the internals of Sinuous very well, but that's all I had to do in my simple clone.

Usage in my example looks like this:

function Counter({ value }) {
  const count = observable(value);

  let div = (
    <div>
      <button onClick={() => count.set(count.get() - 1)}>-</button>
      <input type="number" value={() => count.get()}/>
      <button onClick={() => count.set(count.get() + 1)}>+</button>
    </div>
  );

  onConnect(() => {
    console.log("Hello from Counter! at y-position:", div.getBoundingClientRect().y);
  });

  return div;
}

If you toggle the "show counters" checkbox, you can see this working for a reactive expression - and if you press the "add item" button, you can see it working for map items.

It's a pretty simple solution - what do you think? Am I missing something incredibly obvious here? 🙂

mindplay-dk commented 3 years ago

Huh, it's even simpler than that - we don't even need to check if the node is connected. By the time we call the connect function, we know all the nodes are connected, that's why we're calling it! So it's completely trivial:

let onConnectListeners = [];

export function onConnect(f) {
  onConnectListeners.push(f);
}

export function connect(node) {
  onConnectListeners.forEach(f => f());

  onConnectListeners = [];

  return node;
}

We don't even need to know which node we're dealing with - the node argument is only there for convenience, so you can invoke connect in a return-statement.

Again, this is true in my toy sinuous clone at least, where creation and reactive updates are always distinct - at least so far, with reactive expressions and map being the only things that generate and mount new nodes.

(I've tried several things by now to come up with a simple life-cycle hook for removals, and this seems much harder, since there's no "component instance" with this type of architecture.)