-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support hx-target-5xx syntax in response-targets extension #1629
Support hx-target-5xx syntax in response-targets extension #1629
Conversation
Thanks for compacting this! I'm not into JS, and this extension was my first contribution to any public JS project, so originally I made the control flow a bit unfolded with Therefore, I'm wondering, with the new approatch, in this expression: var attrPossibilities = [
respCode,
respCode.substr(0, 2) + '*',
respCode.substr(0, 2) + 'x',
respCode.substr(0, 1) + '*',
respCode.substr(0, 1) + 'x',
respCode.substr(0, 1) + '**',
respCode.substr(0, 1) + 'xx',
'*',
'x',
'***',
'xxx',
]; Are the elements of the collection bound to This is interesting because having such a mechanism would ensure that each subsequent calculation is performed only when there isn't a prior match. UPDATE after chatting with AI. Seems like the easiest way to achieve that in JS is to have a bunch of fns, like: var attrPossibilities = [
() => respCode,
() => respCode.substr(0, 2) + '*',
() => respCode.substr(0, 2) + 'x',
() => respCode.substr(0, 1) + '*',
() => respCode.substr(0, 1) + 'x',
() => respCode.substr(0, 1) + '**',
() => respCode.substr(0, 1) + 'xx',
() => '*',
() => 'x',
() => '***',
() => 'xxx',
]; Then each result can be obtained with something like But, I don't have a knowledge of how cheap function definitions are in JS, e.g. how much faster it is to create anonymous function in comparison to performing a method call on an object ( |
JavaScript doesn't come with any lazy data structures. If HTMX didn't support IE11 we could look at using generators, but that seems like overkill here. Eagerly evaluating the possible prefixes isn't my favorite thing. I considered using a big chain of Either of those would have hurt readability to some degree (especially with IE11 support ruling out arrow functions). Given that a handful of string operations will have a negligible affect on performance, and that I don't expect this extension to be on a hot code path, I felt it was a worthwhile tradeoff to accept eager evaluation in exchange for readability and explicitness. I'm open to something better though 🙂 |
@spiffytech: I updated my reply while you were replying. :) I see you've also mentioned functions! They have a nice syntax (assuming my example is correct). I thought a bit about how hot is that execution path and in my environment it is not but I could imagine systems with frequent responses, generating multiple lookups effectively. Frankly speaking, I also suggested that because of my personal rule of a thumb to make a collection of elements lazy if there is 8 or more of them and they are not constant values, self-referential forms, or simple k-v lookups. (I think I've seen this approach in Clojure when PS: Actually after couple of years I came up with another rule of a thumb, to implement smaller collections as macros that generate source code with |
The syntax in your example is correct, except for IE11 not supporting arrow functions 😢 HTMX 1 support IE11. I think v2 plans to drop support, fortunately. Under IE11, we'd have to use this noisier syntax: var attrPossibilities = [
function() { return respCode},
...
] Anyway, I measured the speed of the relevant section of code, isolated from the rest of HTMX - just looping through the array. I tested on my laptop, set to aggressively performance-throttle, and exited the loop on average half way through the array, to estimate the value of lazy evaluation. The version in the PR averages 270ns to complete. Converting to an array of functions averages 350ns. So 0.0002 vs 0.0003 milliseconds. So that's a truly negligible difference, and the lazy version comes out slower in JS. My vote is still on current version, as it has less visual noise and cognitive overhead. |
} | ||
} else { | ||
for (let l = targetAttr.length - 1; l > targetAttrMinLen; l--) { | ||
targetAttr = targetAttr.substring(0, l) + '*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the explicitness of your solution, but I'm also curious why you didn't just add:
targetAttrStar = targetAttr.substring(0, l) + '*'
targetAttrX = targetAttr.substring(0, l) + 'x'
targetStr = api.getClosestAttributeValue(elt, targetAttrStar) || api.getClosestAttributeValue(elt, targetAttrX)
Looks great though, love the test and the updated docs. Marked it ready for review so just go ahead and rebase and we'll get it looked at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the explicitness of your solution, but I'm also curious why you didn't just add:
I liked keeping all the possibilities in one place, where they could be understood separately from understanding how the attribute possibilities get processed. Makes it easy for someone to check what's supported.
I don't mind changing it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased!
Some |
Ah, my mistake! I renamed the variable on line 6 in-place without noticing it was already It looks like the |
53bc6d5
to
193d4fd
Compare
Thanks so much @spiffytech |
This PR implements #1564, enabling the
5xx
syntax for the response-targets extension.I refactored the attribute detection logic to make explicit which syntaxes we support. I also added tests to cover all attribute syntaxes.
I updated the docs to note
5xx
as an alternative syntax, leaving the5*
syntax as the prominent recommendation.I noticed that while the original issue discussed the syntax
5xx
(doublex
), the extension currently supports5*
(single asterisk). I hadn't previously noticed the gap between*
's wildcard semantics andx
's placeholder semantics. I elected to make single/double/triple characters work for both syntaxes (to minimize developer surprise), and we can document only most idiomatic5*
and5xx
. If you want me to do something different, let me know.