Skip to content
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

Merged

Conversation

spiffytech
Copy link
Contributor

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 the 5* syntax as the prominent recommendation.

I noticed that while the original issue discussed the syntax 5xx (double x), the extension currently supports 5* (single asterisk). I hadn't previously noticed the gap between *'s wildcard semantics and x'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 idiomatic 5* and 5xx. If you want me to do something different, let me know.

@siefca
Copy link
Contributor

siefca commented Jul 23, 2023

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 if (when there is an exact match) and for (when there is no exact match and we need to check for wildcard matches).

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 attrPossibilities evaluated lazily? If not, is there an easy way in JS to create a kind of lazy list, or another sequential data structure, that wraps each of its elements in a form with delayed execution?

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 attrPrefix + attrPossibilities[i]();.

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 (substr) and making a new string from 2 strings (with +).

@spiffytech
Copy link
Contributor Author

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 if/else blocks. Alternatively, the array could have been an array of functions that each returned a possible prefix.

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 🙂

@siefca
Copy link
Contributor

siefca commented Jul 23, 2023

@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 PersistentArrayMap automagically becomes PersistentHashMap after exceeding 16 elements / 8 pairs.)

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 or expressions which is the fastest. And now I don't have any thumbs left to make rules. :)

@alexpetros alexpetros added the enhancement New feature or request label Jul 24, 2023
@spiffytech
Copy link
Contributor Author

I see you've also mentioned functions! They have a nice syntax (assuming my example is correct)

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.

@alexpetros alexpetros added the ready for review Issues that are ready to be considered for merging label Jul 29, 2023
}
} else {
for (let l = targetAttr.length - 1; l > targetAttrMinLen; l--) {
targetAttr = targetAttr.substring(0, l) + '*';
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased!

@alexpetros alexpetros changed the title Support hx-target-5xx syntax, for maximum compatibility (#1564) Support hx-target-5xx syntax in response-targets extension Jul 31, 2023
@alexpetros alexpetros linked an issue Jul 31, 2023 that may be closed by this pull request
src/ext/response-targets.js Show resolved Hide resolved
@1cg
Copy link
Contributor

1cg commented Aug 1, 2023

Some let and const have found their way into the enhancement. We don't really need to make the enhancements IE compat, but seems like we should at least try.

@1cg 1cg removed the ready for review Issues that are ready to be considered for merging label Aug 1, 2023
@spiffytech
Copy link
Contributor Author

Some let and const have found their way into the enhancement. We don't really need to make the enhancements IE compat, but seems like we should at least try.

Ah, my mistake! I renamed the variable on line 6 in-place without noticing it was already const. I'll take care of that.

It looks like the let you marked is part of the existing code; this PR removes them.

@alexpetros
Copy link
Collaborator

Thanks so much @spiffytech

@alexpetros alexpetros merged commit 66387c0 into bigskysoftware:master Aug 1, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support 5xx syntax for response-targets
4 participants