-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow hx-boost to avoid poisoning the inheritance tree #3243
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
Comments
This feels to me like something that would be ideal to implement as a <body hx-ext="head-support,boost-defaults"
hx-boost="true"
hx-boost-target="#main"
hx-boost-select="#main"
hx-boost-swap="outerHTML transition:true show:window:top">
<nav>...</nav>
<div id="main" hx-history-elt>...</div>
<footer>...</footer>
<div hx-get="/player"
hx-swap="outerHTML transition:false settle:0"
hx-trigger="load[!!window.Alpine], alpine:init from:document"></div>
</body> To implement this you just need to set internalApi on extension init and then use it in the extension htmx onevent you listen for htmx:configRequest and in here check if the request is a boost and if it is call edit: sorry looks like it is htmx:beforeSwap event you need the extension to use and here it can set set evt.detail.target, evt.detail.selectOverride and evt.detail.swapOverride. |
(function () {
let api;
htmx.defineExtension("boost-defaults", {
init: function (apiRef) {
api = apiRef;
},
onEvent: function (name, evt) {
if (name === "htmx:beforeSwap") {
if (evt.detail.boosted) {
const target = api.getClosestAttributeValue(evt.detail.elt, "hx-boost-target");
const selectOverride = api.getClosestAttributeValue(evt.detail.elt, "hx-boost-select");
const swapOverride = api.getClosestAttributeValue(evt.detail.elt, "hx-boost-swap");
if (target) evt.detail.target = target;
if (selectOverride) evt.detail.selectOverride = selectOverride;
if (swapOverride) evt.detail.swapOverride = swapOverride;
}
}
},
});
})(); Quick example as a good starting point. |
I can see that the ergonomics of the extension are better. For purposes of distribution, is this something I could submit to the htmx-extensions repo, reworking the existing tests to be appropriate for an extension? |
Yeah that quick version i posted needs testing and fleshing out. for example you could get it to manually check if target/select/swap is set on the element directly and skip overriding in that situation which would make it better match your example implementation. you could also change it to to const target = api.getClosestAttributeValue(evt.detail.elt, "hx-boost-target") || api.getAttributeValue(document.body, "hx-boost-target"); so that if the closest attribute with inheritance is not found maybe because inheritance is disabled then it will fall back to body value for submitting the finial extension for distribution the current guidelines are unless it is a real core extension or already in the htmx-extensions repo then new custom extensions should be hosted somewhere like your own github repo and then a link to the repo and its readme can be submitted as a PR to the htmx readme: https://github.com/bigskysoftware/htmx/blob/master/www/content/extensions/_index.md You can see many examples here of externally hosted patterns to guide you. It can be as simple as a .js file and a readme or you can do what I did and just fork htmx-extensions repo and customize it for your extension and reuse whatever you want from here. |
While I’m open to any approach to improve it, I do feel that code requiring vendoring can be off-putting, even if I'm the author! A large set of platforms will use NPM to handle JS and I personally use WebJars with Java which also relies on NPM. My intention with this code wasn’t to introduce a new feature, but rather to address significant issues with the existing one. I truly appreciate the time you’ve taken to review it. Perhaps I’m overly passionate about this since it’s “my” feature, but I wonder, is the only path forward to publish my own NPM package? Is Design notes: |
Adding small bits of javascript to a public folder and linking them with a script tag is an option available in all platforms and is often simpler and easier to maintain than javascript management solutions. Yeah discussions on the future of htmx extensions would probably be best in Discord. I'm just a nobody that is just repeating the guidance others have given. There is an issue that they let anyone submit an extension to the htmx-extensions repository in the past and now they have a mix of core extensions they want to support and develop plus community ones they are now having to support as well so it was seen as easier to instead push for new community extensions to be linked to but not hosted and maintained by the htmx team. I think the goal is now to kind of lock down htmx as basically feature complete now and avoid adding more features and things to the htmx core and just mostly do bug fixes and optimization only. But that means extension development and support will probably take more of the focus going forward. As to the extension you can check the swap/target/select values of the triggered elt by using the same code as core htmx does and also use |
Although it may be more work for the author, I think it can be less work for the consumer. Leaving the option open for grabbing the code can be helpful, but it could confuse and put off people expecting to use dependency management and your one package refuses to offer it. I think publishing the package gives people the signal that you're not just creating abandonware. |
One thing to note @scrhartley is that npm fully supports github as a package source out of the box. This means you don't actually need to publish to npm you can just host it on github as long as you customize the package.json how you want based on the existing examples in the htmx extension repo. Then you can just type
and it will search for a github user name scrhartly and project name htmx-ext-boost-attrs with a tag or branch name of v2.0.1 and install that as expected |
That's great news. Hopefully along with WebJar's GitHub support it means everything's covered for me. |
I'm now investigating if I need a dist build step in order to also provide an ESM module and maybe a minified build, or if they're not needed. |
I've found this extension has an unusual gotcha. |
You can reuse and copy all the dist packaging and esm steps from the htmx-extensions repository as these should all work exactly the same for you. https://github.com/bigskysoftware/htmx-extensions/blob/main/scripts/dist-all.sh For some extensions like this one they will require the hx-ext set on the body for them to fully work properly and this should be called out in the extension documentation as the recommended deployment method. |
I'm now leaning towards not having that special casing of looking at body when inheritance is disabled. |
During the development of this extension, I've noticed that the limitations of extensions is pushing me into having to create work-arounds; I'll outline one of them. Currently, after each event, htmx calls Here's the call order:
This is something that could be added in a backwards compatible manner. |
For a more concrete explanation regarding the previous comment, let's consider the current WIP: (function() {
let api
htmx.defineExtension('boost-attrs', {
init: function(apiRef) {
api = apiRef
},
onEvent: function(name, evt) {
if (name !== 'htmx:beforeSwap' || !evt.detail.boosted) return
const elt = evt.detail.requestConfig.elt
const swapOverride = api.getClosestAttributeValue(elt, 'hx-boost-swap')
const selectOverride = api.getClosestAttributeValue(elt, 'hx-boost-select')
const targetOverride = api.getClosestAttributeValue(elt, 'hx-boost-target')
const headers = evt.detail.xhr.getAllResponseHeaders()
if (swapOverride && !/HX-Reswap:/i.test(headers)) evt.detail.swapOverride = swapOverride
if (selectOverride && !/HX-Reselect:/i.test(headers)) evt.detail.selectOverride = selectOverride
if (targetOverride && !/HX-Retarget:/i.test(headers)) {
const target = targetOverride === 'this' ? api.findThisElement(elt, 'hx-boost-target') : api.querySelectorExt(elt, targetOverride)
if (target) {
evt.detail.target = target
} else {
api.triggerErrorEvent(elt, 'htmx:boostTargetError', { target: targetOverride })
return false
}
}
api.triggerEvent(evt.detail.target, 'htmx:beforeBoostSwap', evt.detail)
}
})
})() So why's there an |
Unfortunately browser events are not free from a performance point of view and the onEvent extension events is fired very very often and doubling the number of events extensions fire would cause a doubling of this performance cost. I think the solution of re issuing a second event like your beforeBoostSwap is the best solution as it would seem wierd to me to try overloading the existing beforeSwap event for what is really a new purpose that will only be used by users of this extension. Another idea to investigate is that extensions do not have to only use the onEvent() event hook point as during init you can register your own htmx event listener on say body. This allows it to fire as a normal htmx event listener order and also to function on body events even if the extension is not targeted to the body. So you have to be aware this does change the way extension inheritance works a little. I'm not sure if this will help with your issue though as body event listeners may fire after listeners attached to child elements and the order the event listeners are registered is also important. |
I was suggesting a new hook, not a new browser event, which most of the time would be an empty function call and hopefully the JIT can optimize that. Since it's not interacting with the DOM, it should be fast either way. I didn't see the re-using the existing beforeSwap event as it being for a new purpose. It's still called for boosts and users can (and should?) differentiate by checking I did consider event delegation but I think bubbling you'd end up with the same issue of it firing after all the other listeners, even if it was registered first. i.e. I assume it triggers all the listeners at each level before bubbling up to the next, rather than bubbling per listener. |
Hey @MichaelWest22, I've put my work in a repo now and so I hope it's in the best state it can be for feedback. Thanks for all your help, suggestions and discussion with this, it's been really satisfying to see everything get to where it is now. |
@scrhartley yeah that extension looks fine to me but I'm not an expert in boosting to really test it out. I've just been doing some experimentation on how we could possibly extend the htmx extension system to allow more advanced extensions. dev...MichaelWest22:eval-ext Just playing around at this point with ideas really. Added in the option for global extensions that only consume the internalAPI and a new global event hook that fires before the main events generate so you can get all events globally and not just the ones your hx-ext is installed on which would be useful for some extension types. you could use both the old and new extension hooks in the same extension if needed as well. This demo also has a refactor of maybeEval to then allow my test extension to replace maybeEval with a more advanced one that converts htmx eval scripts into proper scripts which i was thinking of using in a possible CSP extension. The other possible extension hook point this demos is doing kind of function proxying via the internalAPI by backing up and replacing one of a select few proxyable functions. not sure what the best way is going to be to allow advanced extensibility in htmx yet. |
As I argued for above, the fact that the global event hook fires before the event is useful and so there would be a motivation to use it even if I'm not relying on its global nature. As an extension it would be odd that I could access the before event hook without requiring the extension to be enabled, while for the after event hook it would need to be. I imagine it could lead to some strange behaviors, but it would feel limiting if global extensions were disallowed the after event hook. I'm unsure about global hooks as I don't know if the current need to enable extensions is for the user's benefit and when this can be waived. P.S. It looks like you haven't got to the part for |
Having thought about it a bit: |
Yeah I'm just testing a few options to see what works and what doesn't. my simplistic array of hooks idea I've found from testing to not work well if the extension script runs twice so its not ideal. My idea was that the before global hook would be all you would use for global extensions and you would likely not need to use the onEvent later hook as an extension like your boost attrs if it were global could just use this one new hook I think. I was thinking of the non event functions that a global extension could optionally still use but yeah it is not ideal when the scope of the extension changes between different functions and as you say could cause confusion. Carson mentioned wishing extensions had just always been global to keep them simple and enable them via a meta config tag and maybe that is another good option. I've just updated dev...MichaelWest22:eval-ext with an example of how we could have isGlobal boolean in the extension to register a global extension and minor updates to getExtensions() so that global extensions work as you would expect and these don't need hx-ext attributes set anymore to function. I have the new preEventHook currently as a global only event function because doing a full getExtensions() evaluation function to walk up the tree to scan for hx-ext for every event twice would be expensive. maybe this could be optimized a bit though. One issue is that with this change anyone can just make all their extensions global in their JS extension and override the whole htmx website without applying any attributes and its now not obvious at all which extensions are allowed to apply. We could have a htmx.config.enableGlobalExtensions config that you set via the htmx config meta tag in the head and block global extensions not in this list. But extension writers would just abuse this by setting htmx.config to enable themselves so maybe having it query the meta tag directly or create a new meta tag just for this to enforce the restriction of global extensions. |
Although the DOM tree could potentially change between the before hook and the after hook, I think this could be confusing and so it would make sense to use the same set of extensions for both. function withExtensions(elt, toDo, extensions = getExtensions(elt)) { or function withExtensions(elt, toDo) {
const extensions = Array.isArray(elt) ? elt : getExtensions(elt)
} |
@MichaelWest22 When I posted on Discord, the question of whether boosted links and forms might have different requirements came to light. I've only used |
I don't have much experience with boosted forms either sorry. But my gut feeling based on looking at the way you proposed this extension idea originally I don't think links and forms would need to behave differently. You might want to customize a form or two to boost differently maybe on a case by case basis and this could be done with attributes set on the form or its parent. You may want to just leave having forms and links use different attributes out at first and only add this if other users ask for this. You could have But as a seperate issue, thinking about it the way boost normally works is if you wanted to override one of the attributes for a single boosted thing you would just use normal htmx attributes placed on it. But your current extension actually blocks the three core attributes target/select/swap working at all on boosted elements. The original goal i think was to allow non boosted elements to not consume the boosted attributes but it may be confusing to some that it also blocks boosted elements using normal htmx attributes. Should it maybe be doing: const swapOverride = api.getClosestAttributeValue(elt, 'hx-swap') || api.getClosestAttributeValue(elt, 'hx-boost-swap')
... Which will then check the normal attributes and if present use them instead and only apply the boost ones as the fallback. You could also consider defining your own custom htmx config value for your extension by checking for htmx.config.boostAttrsOverride or similar and if this is manually set true then you toggle the mode of checking the base attributes first or not. User can then just add an example of how to set this config value with a normal htmx config meta tag when needed. |
The current top level example allows you to think about boost swaps and normal swaps separately. The extension is trying to solve the problem of inheritance and the interaction of two sets of attributes would seem to be a source of confusion to move away from. How about instead doubling down on the overriding and always doing it? |
hx-boost
is useful, but can easily cause issues when used together with regular swaps.Details
Here is the originally motivating example:
Because I'm using
hx-boost
together withhx-target
,hx-select
andhx-swap
,everytime I want do a non-boosted swap, I have to explicitly set them to undo the boost values.
(Most obviously, needing to use
hx-target="this"
andhx-select="unset"
.)These workarounds can quickly become tedious if you have more than a few swaps and aren't obvious to a lot of users.
Even when not using target/select, swap modifiers impact any normal swap where
hx-swap
isn't explicitly specified.Note: I can't just use
hx-disinherit
high up in the tree sincehx-boost
generally relies on inheritence to work.Note: Perhaps this particular situation could instead use
hx-preserve
, but it doesn't affect the underlying argument.Attempted solution
Instead of being forced to use attributes to set these values, I propose boost-specific defaults that can be specified in the config.
These would only apply for boosts and would be ignored if their equivalent attribute was found and hence would prioritize locality of behavior. An alternative approach of prioritizing config values would further separate boost and regular swaps, but would require more thought.
Here is an example implementation that adds
defaultBoostSwap
,defaultBoostTarget
anddefaultBoostSelect
to the config (feel free to bike-shed).Note: This implementation needs to be updated to have the correct value for
htmx:targetError
but is waiting for PR #3178 to be merged first.Note: Even though
hx-history-elt
may often be used in conjunction, currently no effort is made to avoid still having to specify it as an attribute.If the idea for this feature is approved then I could start work adding updates for the docs.
The text was updated successfully, but these errors were encountered: