Unexpected brace wrapping for `hx-vals`
Closed this issue · 5 comments
I want to make an element to include dynamic values generated via javascript. The suggested way to do this is through hx-vals=... attribute. Here's an example use case where I want to implement a list of toggles whose values are aggregated before sending to the server:
<ul hx-get="..." hx-vals="js:aggregateVals()" hx-trigger="click from:li">
<li data-enabled="1" data-id="1" onclick="this.dataset.enabled=1-(+this.dataset.enabled)">1</li>
<li data-enabled="0" data-id="2" onclick="this.dataset.enabled=1-(+this.dataset.enabled)">2</li>
<li data-enabled="0" data-id="3" onclick="this.dataset.enabled=1-(+this.dataset.enabled)">3</li>
</ul>
<script>
function aggregateVals() {
const enabledEntries = Array.from(document.querySelectorAll("li")).filter(x => +x.dataset.enabled).map(x => x.dataset.id).join(",");
if (enabledEntries === "") {
return {};
} else {
return {entries: enabledEntries};
}
}
</script>My intention here is to skip the query parameter altogether if no entry is enabled. And if any entries are enabled, the query can be compactly encoded like &entries=1,2,3.
However, according to the current implementation, hx-vals will implicitly wrap the value after js: with braces, so the actual evaluated expression becomes {aggregateVals()}. I tried to get around the problem with this:
<ul hx-vals="js:...aggregateVals()" ...>It works as expected but the syntax looks very unnatural to me. I suppose the implicit braces may be added to hx-vals so it's compatible with the now deprecated hx-vars attribute. However, since this implicit syntax is never mentioned in the documentation of hx-vals, I suppose such usage is not encouraged. If so can we remove it in exchange for a nicer syntax for the more dynamic use case as I demonstrated?
Yeah I think the {} wrapping has been added to make sure every return is in the right object format to be processed and prevent non object values being provided. The example documentation gives hx-vals='js:{myVal: calculateValue()}' which forces it to provide a value of myVal with a dynamic value but you are wanting the option to not provide the property when it has not been set. Someone just raised a possible fix to improve hx-vals in #2884 and it seems that an improved version of that fix could make your use case easier as it could be changed to strip undefined or maybe null properties allowing you to simplify your code by having hx-vals='js:{"entries": getEntries()}' and getting getEntries to return undefined or a value and it then only sends that parameter when it's valid.
I previously tried hx-vals='js:{"entries": getEntries()}' with getEntries() returning null/undefined but found it didn't work, which is exactly what's report in #2884. Thanks for the reference.
But that said, I believe the bug in #2884 is independent from the feature described here. If we open up the ability specifying values via a js function, we can support much more than what's achievable with static keys. And that's already doable with the current version of htmx as I demonstrated, just with an awkward syntax like js:...fun().
However, since this implicit syntax is never mentioned in the documentation of hx-vals, I suppose such usage is not encouraged.
Initially, I would have agreed with what you're saying here given that the doc says the following:
the value of hx-vals must be valid JSON.
Then, there is the eternal issue of backward compatibility. We introduced breaking changes in htmx 2, and unfortunately, this implicit syntax wasn't one of them. So we have the issue of "if we change this now, we might break code that had been correctly migrated to htmx 2 and would suddenly not work anymore", which we cannot afford (that is, without releasing a new major update).
What we use to determine whether it's an acceptable fix or not, is the result of the test suite. The test suite reflects the features we guarantee at a given time, that we could only break with a major release (tbh, htmx 3 is very unlikely to ever happen).
Unfortunately, the test suite seems (did you run it locally btw? I saw you ticked the box in the PR template, is it a pipeline issue or do the tests also fail locally?) to indicate that this indeed breaks, even though a deprecated feature (hv-vars), still a feature that we provide support for nonetheless.
That could still be discussed though (everything can be discussed after all), but the issue I see is:
- Cons: introduce a breaking change (we never know how many people might be affected ofc, but technically it still is a breaking change)
- Pros: avoid using a js spread operator
For the latter, as you said
just with an awkward syntax like js:...fun().
Maybe that's just a "each to his own" situation, but that awkward syntax you're describing is the spread operator, that I must admit I'm not sure what's so awkward about it?
If you want to make it clearer, you could also explicitly put the braces to make it even more explicit, such as
<ul hx-vals="js:{ ...aggregateVals() }">
...
</ul>I know it can be frustrating to get a change refused, but I hope you'll understand that weighing up the pros and cons here don't really go in favor of this change!
Again, this greatly depends on the test suite's results @shouya, so if you want to check it out, you'll want to
- retarget your PR to
devfirst as per our contribution guidelines, which btw includes a CI fix that isn't onmasteryet (which is why I'm asking if it works locally or not, the CI is broken on master atm) - run the test suite, and see how it goes
I agree with the pros and cons you listed. Indeed, although I think the implicitly addition of brace is sort of gratuitous, I see it does not worth it to break compatibility promise just for the sake of an arguably cleaner syntax. On a hindsight I admit I'm a bit too aggressive in pushing the issue when I could only resort to a hackish solution.
I agree with the current way of enforcing the braces. It's a way to suggest the attribute only accepts json/js objects. I see that js:{ ...aggregateVals() } is cleaner and admit that's better than a plain js:aggregateVals() for this reason.
Looking back with the root of my confusion is that there seems to be no way of providing dynamical keys to the request params, at least I find nothing in the documentation. So when I searched through the source and cobbled up this unintended usage, I had to call it a hack.
So how about we consider this spread operator officially supported? By which I mean adding an example in hx-vals documentation for hx=vals="js:{...generateKeys()}" and add a test case for it?