Skip to content

Conversation

botandrose
Copy link
Collaborator

@botandrose botandrose commented Feb 15, 2025

Overview

I think Idiomorph could benefit from a plugin system.

The idea here to is make it easy to encapsulate useful ideas and integrations in individual files or packages, e.g. "idiomorph/ignore-value", "idiomorph/htmx", "idiomorph/ignore-active-value", "idiomorph/turbo", etc. Multiple plugins should be able to be easily composed.

Example

Idiomorph.addPlugin({
    name: 'logger',
    beforeNodeAdded: function(node) {
        console.log('Node added:', node);
    },
    beforeNodeRemoved: function(node) {
        console.log('Node removed:', node);
    },
});

Benefits

  • Potentially slimming down core by extracting less-frequently-used options into plugins. For example, ignoreActive or ignoreActiveValue might be good candidates for this.
  • Allowing transparent composition of different callback-related concerns. Right now, all lifecycle callbacks are singletons. So if one wants to register multiple beforeNodeMorphed callbacks, for example, they'd have to manually compose the individual functions themselves.
  • Enabling experimentation and third-party integration outside of core. This could enable third-party npm packages to be published that provide an Idiomorph integration or modification, without requiring us to merge anything into core.
  • May be internally useful for core, for separating concerns.

Questions

  • How do we want the interface to be? Should we name it defineExtension to line up with its cousin, htmx?
  • Can this system support require, import, and <script src>?
  • What if we don't control the Idiomorph call itself? For example, Idiomorph is a hidden implementation detail of Turbo. Could a user of Turbo add a plugin to Idiomorph, even though it's buried within the guts of Turbo?

So, is this a good idea? What do we think about the API?

@botandrose botandrose changed the title [DRAFT] Proof-of-concept for a plugin system Add a plugin system Feb 16, 2025
@botandrose
Copy link
Collaborator Author

I've resolved the known issues, so I'm graduating this to a full PR, and its ready for review.

@botandrose
Copy link
Collaborator Author

So here's the actual use-case that first got me thinking about a plugin system for Idiomorph: Composition (or lack thereof) of multiple beforeNodeMorphed callbacks. Below is the code currently in Turbo for beforeNodeMorphed:

  beforeNodeMorphed = (currentElement, newElement) => {
    if (currentElement instanceof Element) {
      if (!currentElement.hasAttribute("data-turbo-permanent") && this.#beforeNodeMorphed(currentElement, newElement)) {
        const event = dispatch("turbo:before-morph-element", {
          cancelable: true,
          target: currentElement,
          detail: { currentElement, newElement }
        })
        return !event.defaultPrevented
      } else {
        return false
      }
    }
  }

As you can see, there are three separate concerns all mooshed together in there:

  • The data-turbo-permanent implementation
  • The private #beforeNodeMorphed property is how Turbo is currently composing callbacks. This is used specifically for reloading <turbo-frame>s on morph (rather than morphing its contents), implemented elsewhere.
  • Dispatching a custom turbo:before-morph-element event for allowing Turbo's consumers to tap into the beforeNodeMorphed logic themselves. Another strategy for enabling composition, ultimately.

Now, in my actual app, I have implemented an optimization: data-turbo-version. If this custom attribute is present on the old and new element, and the value of this attribute is not newer in the new element, the morph is prevented. This is critical for keeping the app responsive even though we're constantly pushing huge documents down the wire. Here is the implementation for that:

if (currentElement.id && currentElement.hasAttribute("data-turbo-version") && currentElement.id === newElement?.id && currentElement.getAttribute("data-turbo-version") >= newElement?.getAttribute("data-turbo-version")) {
  return false
}

So, here's the motivating question: where do I put this code without forking Turbo or Idiomorph? I actually could most likely tap into Turbo's turbo:before-morph-element event, but there aren't corresponding events for the other Idiomorph hooks.
Secondarily, it seems clear that Turbo is already struggling a bit with the lack of callback composition, even before I wanted to add in my own bit externally.

So, I submit that the plugin system I'm proposing would clean all this up nicely.

@1cg
Copy link
Contributor

1cg commented Feb 21, 2025

Two questions:

  • If we add this, would it make sense to remove the old callback system. We are pre-1.0 so now's the time to do it, and it seems like this replaces it.
  • What's the perf hit?

If the perf hit isn't too bad I'd prefer to remove the callbacks and just use this instead.

@botandrose
Copy link
Collaborator Author

botandrose commented Feb 21, 2025

@1cg Hmm yeah good point about the API considerations. If this were merged as-is, we would have three different methods of configuring Idiomorph:

  1. Idiomorph.defaults.merge({ ... })
  2. Idiomorph.morph(..., { callbacks: ... })
  3. Idiomorph.addPlugin({ ... })

Let me think some more about how we might consolidate this API.

Re: perf, I haven't looked into that at all yet, because the API is still up in the air. But maybe it would be informative to get some numbers at this point.

As an alternative idea, what do you think about ditching callbacks entirely and going with dispatching idiomorph:beforeNodeMorphed et al events? I think they might be a bit better suited to composition.

@botandrose
Copy link
Collaborator Author

@1cg The more I consider the events idea, the more I like it.

  1. It would reduce the API surface area, not increase it
  2. Events are naturally composable

I'll explore it on its own branch, and we can compare.

@1cg
Copy link
Contributor

1cg commented Feb 21, 2025

I very much prefer events, but didn't use them initially due to perf considerations. They have to bubble to be useful and bubbling means for a tree w/ N nodes you are going to trigger N*lg(N) event handlers w/N newly created events (iirc), w/fairly high constants as well. We can look at it again though: I definitely think that's the cleanest way to integrate independent pieces of software (see htmx, fixi, hyperscript, etc.)

@botandrose
Copy link
Collaborator Author

@1cg Ah yeah, that makes sense. We might be able to get around the perf issue by turning off bubbling and simply triggering them on the root node?

Either way, with the new tachometer perf system, its super simple to compare the perf of branches to baseline. I'll spike all this out, and come back with some hard numbers.

@1cg
Copy link
Contributor

1cg commented Feb 21, 2025

triggering it only on the root w/o bubbling is interesting, but in my experience the cleanest way to integrate things in general is to allow people to listen on document which requires bubbling. I can imagine a situation where you have a bubbling event that allows document to hook in event listeners on the element in question, but that starts to get a little complicated.

Let's see what the perf looks like and go from there. Strongly favor events if at all possible though!

@MichaelWest22
Copy link
Collaborator

I tested with dispatching events just to the document element and attaching all listeners to document as well and it works just fine like this and you don't then have to worry about bubbling. Also I found the beforeNodeAdded event I had issues with when trying dispatching to the node itself and using bubbling because the node is not yet on the DOM! Maybe you can message the parentNode but this seems hacky and unreliable to me and I think just sticking to document may be a good option to simplify things. Avoids the support hassles when node trees lose event handlers during morphing.

I think the current callback solution is probably the most performant option but as Micha has pointed out it is not obvious on how to add complex composability with the callback. However technically callbacks are more customizable and composable than events it is just more complex to use in practice. Also from the use cases i've seen for datastore and turbo their is only one event that is really used in practice which is beforeNodeMorphed as for almost all real world cases this is where the most useful decisions can be made. So forcing all users to flood the browser with all events that are never being listened to for every internal action could be a bit of a waste. The current noop callbacks are probably very efficient in this regard. I wonder if we could adopt a hybrid of some kind where we keep the callback solution as it is now but provide the ability for end users to if they choose select one or more of the existing callbacks and convert it into a customEvent emitter. I think it should only take a few lines of callback code to send the event to document and return the result of the event back. This could just be a documented recommended example callback pattern that turbo could then implement and then they could move all the composable use cases to event listeners.

Idiomorph.defaults.callbacks.beforeNodeMorphed = (node) => {return document.dispatchEvent(
        new CustomEvent("im-before-node-morphed", {
          cancelable: true,
          detail: { node },
        }),
      );};

One idea I also had was to add a eventCallbacks optional string config value which is just a comma seperated list of callback names to emit as events so the end user can then choose just the one-two events they want to use event listeners for and take the perf hit just on these events while other users get no negative impact.

So i mocked this up here:
main...MichaelWest22:eventCallback

You just set the callback names and it emits the events as kebab case for compatiblity using beforeNodeMorphed -> im-before-node-morphed custom event name. It works by just wrapping the existing callbacks with the additional event listener so any existing callbacks in use keep working 100% as before to maintain full backwards compatibility. i ran perf tests and found no real change if no events listed and the cost to call both the custom event and the callback together did not slow things down more. But adding the event dispatch did cause a large slowdown especially if we fire it on all 7 events.

Found a larger 20-40% slowdown with all events and bubbling in use
Found all events but with no bubbling by using document it was maybe 5% faster but still almost as slow
Found that just emitting the beforeNodeMorphed event to document was around 10% slower

idiomorph-full-event-bubbling.txt
idiomorph-full-event-to-document.txt
idiomorph-one-event-to-document.txt

@botandrose
Copy link
Collaborator Author

botandrose commented Mar 3, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants