-
Notifications
You must be signed in to change notification settings - Fork 44
Add a plugin system #114
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
base: main
Are you sure you want to change the base?
Add a plugin system #114
Conversation
I've resolved the known issues, so I'm graduating this to a full PR, and its ready for review. |
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 = (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:
Now, in my actual app, I have implemented an optimization: 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 So, I submit that the plugin system I'm proposing would clean all this up nicely. |
Two questions:
If the perf hit isn't too bad I'd prefer to remove the callbacks and just use this instead. |
@1cg Hmm yeah good point about the API considerations. If this were merged as-is, we would have three different methods of configuring Idiomorph:
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 |
@1cg The more I consider the events idea, the more I like it.
I'll explore it on its own branch, and we can compare. |
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.) |
@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. |
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 Let's see what the perf looks like and go from there. Strongly favor events if at all possible though! |
I tested with dispatching events just to the 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 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: You just set the callback names and it emits the events as kebab case for compatiblity using Found a larger 20-40% slowdown with all events and bubbling in use idiomorph-full-event-bubbling.txt |
Wow, amazing work, Michael! Thank you for taking the time to explore this
so thoroughly. I've been in crunch mode trying to get bardtracker.com over
the line into beta, lately. But I'm very excited to take a look at the work
you've done in more detail!
…On Sun, Mar 2, 2025 at 10:16 PM MichaelWest22 ***@***.***> wrote:
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
<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
<https://github.com/user-attachments/files/19047301/idiomorph-full-event-bubbling.txt>
idiomorph-full-event-to-document.txt
<https://github.com/user-attachments/files/19047302/idiomorph-full-event-to-document.txt>
idiomorph-one-event-to-document.txt
<https://github.com/user-attachments/files/19047303/idiomorph-one-event-to-document.txt>
—
Reply to this email directly, view it on GitHub
<#114 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEH36ACNIPWACWWWWMDCT2SPJTDAVCNFSM6AAAAABXGSPU26VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGIZTONJXGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
[image: MichaelWest22]*MichaelWest22* left a comment
(bigskysoftware/idiomorph#114)
<#114 (comment)>
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
<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
<https://github.com/user-attachments/files/19047301/idiomorph-full-event-bubbling.txt>
idiomorph-full-event-to-document.txt
<https://github.com/user-attachments/files/19047302/idiomorph-full-event-to-document.txt>
idiomorph-one-event-to-document.txt
<https://github.com/user-attachments/files/19047303/idiomorph-one-event-to-document.txt>
—
Reply to this email directly, view it on GitHub
<#114 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEH36ACNIPWACWWWWMDCT2SPJTDAVCNFSM6AAAAABXGSPU26VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGIZTONJXGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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
Benefits
ignoreActive
orignoreActiveValue
might be good candidates for this.beforeNodeMorphed
callbacks, for example, they'd have to manually compose the individual functions themselves.Questions
defineExtension
to line up with its cousin, htmx?require
,import
, and<script src>
?So, is this a good idea? What do we think about the API?