bigskysoftware / idiomorph

A DOM-merging algorithm
BSD 2-Clause "Simplified" License
640 stars 32 forks source link

open to callbacks? #3

Closed leastbad closed 1 year ago

leastbad commented 1 year ago

StimulusReflex folks here. This is a really exciting vision, kudos for your approach.

I happen to agree that sacrificing a bit of performance for much more reliable outcomes is a win. We actually keep a checklist of weird morphdom shit and boy, would I love to be able to delete it.

We're considering jumping ship for a few reasons beyond edge case gotchas, mostly relating to callbacks. There is a onBeforeElUpdated callback, but they don't have anything for text nodes, or text <-> element conversions with incompatible types. We use onBeforeElUpdated to allow SR developers to mark elements as "permanent" via a data-reflex-permanent attribute. It works for element nodes, but people are constantly asking us why morphing a string can't be prevented.

Are you open to providing a callback that would halt all morphing for an element and its children if it returns false?

The other thing is a nice-to-have, and that's a callback for when all nodes have been processed. This has been extensively discussed but nothing seems to have solidified.

Finally, as a bonus round question: are you open to implementing the equivalent to morphdom's childrenOnly option?

1cg commented 1 year ago

Yes to all of this.

Very willing to sacrifice perf (especially rare perf) for better flexibility. The algorithm isn't very big, so if you see places you like a callback, just let me know.

Seems like a callback object is the right thing.

And, yeah, children only is going to be an option (I want to adopt the outerHTML, innerHTML terms from normal DOM ops)

1cg commented 1 year ago

One thing I'd like to keep about this algorithm is understandability. I can barely follow the morphdom algorithm, so I really want this one will be as obvious as possible.

leastbad commented 1 year ago

First, thank you for replying. This might already be the best thing about idiomorph, sadly: you seem excited about what your potential users actually think. You want to be part of the solution! How wild is it that this is radical?

I have spent enough time with the morphdom codebase that I can comfortably navigate it, and even confidently improve it. It's actually reasonably well documented with in-line notes about what each section does. If you can get into the zone it's possible to get a handle on how it works, but like all things which start simple, some of the stuff that wasn't anticipated on day-one has contributed to making things more convoluted than they need to be. The upside is that the morphdom source can offer us a checklist of cases that we have to make sure are covered in idiomorph (and SR!) After all, we can sacrifice performance but we really want to avoid being less good.

Speaking of performance, I would suggest that it's very important you generate some performance metrics for direct comparison to morphdom sooner than later. It's ace that you got out in front by stating that idiomorph is slower, but the next step is to quanify how much slower we're talking about. There's a difference between 30%, 200% and 3000%, even to someone on your side. The bad answer isn't being X% slower, it's not being able to offer an educated projection of what X is. If we're asking folks to make important tech decisions that could impact their reputation and employment, it's fair to give them context for what they are championing. The only way a champion can do so effectively to their team is to arm them with facts, good and bad.

childrenOnly, innerHTML, insideBits... call it whatever you like! We just need the functionality, as SR morphs children of elements. Although I would say that if you call something "innerHTML" but you're actually morphing, that might legitimately end up confusing folks... especially if, like SR, you employ morphdom AND (real) innerHTML depending on the scenario.

I'm not sure what you mean by a callback object; could you expand on this? FWIW, I recommend that you stick to the general spirit of how morphdom implements its callbacks, with a default () => {} no-op function that you can pass in a function to override it. Everything continues as normal unless the developer provides a function that returns false.

As I said, I would just like to see the onBeforeElementUpdated equivalent be updated to execute before all node types, not just element nodes that are "compatible". As I said, today morphdom only fires for "compatible" element nodes. Great in the lab, but in the wild, people morph everything.

1cg commented 1 year ago

i spent some time today cleaning up the code. I ended up porting it a functional style instead of using classes, I just don't like JS classes. I also added the AMD insanity from htmx, which I barely understand, but it works.

as far as perf, I took out the array copy I was doing and, as near as I can tell, the perf is pretty close to morphdom. I have two perf tests:

https://github.com/bigskysoftware/idiomorph/blob/main/test/perf.js

and idiomorph is consistently around morphdom (faster for the HTML 5 demo for some reason). I'll try to come up with some psychotically bad HTML structures for the idiomorph algorithm.

for callbacks, I'm thinking something like what you are describing:

Idiomorph.morph(node, newContent, {
  onNodeAdd:function(newNode) {
    doSomethingWith(newNode);
  }, ...
  );

If you want to take a look at the current code (I think the algorithm is a lot clearer now) and suggest callbacks, I'm happy to defer to your expertise on it.

1cg commented 1 year ago

Added some basic callback support, but still looking for feedback on how these might be used. I see that ideomorph lets you bail out of a removal, etc. so any feedback on that would be appreciated

https://github.com/bigskysoftware/idiomorph/blob/7ca9e34f76a7d68b6283f5aab802380986d14db7/src/idiomorph.js#L71

leastbad commented 1 year ago

I'm super excited to learn the codebase and go over your changes. Just might take me a few days; fighting lots of small fires.

1cg commented 1 year ago

No worries.

I just finished up supporting both outerHTML and innerHTML morphing, with some tests.

Will continue adding tests for corner cases and porting over tests from the other libraries to make sure we are doing the right thing, but it's looking pretty good to me right now. The code all firmed up and the perf is within spitting distance of morphdom.

lmk if you see anything or want changes. I'm gonna try to recruit some people to try it out in the htmx community.

1cg commented 1 year ago

Hey @leastbad checking in on if you've had a chance to look at the code. I cut a few early point releases and I think the algorithm is reasonably complete at this point, mod the inevitable bugs/corner cases. Would like to make sure I've got the callbacks you want for your needs.

leastbad commented 1 year ago

I appreciate your enthusiasm and the speed at which you're progressing is exciting, but I cannot work as quickly as you without compromising my existing responsibilities and commitments. I was hoping to get to this on a "sometime in the next week or so" timeline, and I urge you to consider that this is not a library to be rushed.

If you/we want to present a legitimate alternative path to morphdom, we owe it to the users of the frameworks that will adopt it to be willing to agonize over early architectural decisions in the earliest days a little bit. A bit of patience today could have dramatic impacts for years to come.

In the meantime, I'm going to do my very best. I got this nudge from GitHub moments after promising someone else to lock myself in a room and not come out until our installer is fixed. OSS is hard.

1cg commented 1 year ago

Oh, no worries. I'll leave this issue open and get to it when you get to it.

Good luck, and deep breaths...

1cg commented 1 year ago

Closing out this issue, there are a lot of callbacks now. If you need different behavior from any of them just let me know and we can get it worked into the library.

davidalejandroaguilar commented 1 year ago

Thanks for the callbacks @1cg , though the problem is they're just called, with no possibility of stopping the behavior if they return false, as it's done on morphdom.

This should be an easy change, but I wonder if this was done on purpose.

1cg commented 1 year ago

it wasn't done on purpose, and we should allow people to opt out of merging/removing/adding/etc. w/ the call backs.

Want to make a pull request?

davidalejandroaguilar commented 1 year ago

@1cg Noted, thanks. Yeah sure, will do.

1cg commented 1 year ago

heya @davidalejandroaguilar checkin in on this if you are interested in adding this functionality

davidalejandroaguilar commented 1 year ago

@1cg Sorry for the delay, this past month was hectic. I opened the PR here https://github.com/bigskysoftware/idiomorph/pull/6