htmx icon indicating copy to clipboard operation
htmx copied to clipboard

Support multiple extended selectors for hx-include, hx-trigger from, and hx-disabled-elt

Open Telroshan opened this issue 1 year ago • 3 comments

Description

hx-include will include all elements matching the given selector, but currently only supports 1 extended selector at a time, meaning that

<div hx-get="/" hx-include="input, #test">

will work, and include values from all elements that match any of these 2 selectors.

Using 1 extended selector works well, like

<div hx-get="/" hx-include="find input">

will retrieve the first child input's value, and works as expected.

However, the following won't work:

<div hx-get="/" hx-include="find input, previous input">

Htmx will currently simply parse the first find keyword and use input, previous input as the selector to test against previous elements. So, the second part previous input won't work at all, as its previous keyword won't even be interpreted by htmx. It will instead be processed as a standard selector, meaning searching for an input under a previous tag (which ofc, doesn't exist)

Explanations

This PR does the following:

  • Break the selector string by comma, to retrieve individual selectors
  • Check for special extended keywords, at the start of each of those selectors, and combine the results
  • Standard selectors are joined back at the end (effectively resulting in the initial selector with any extended selector removed from it) and passed to querySelectorAll as it used to.

Corresponding issue: #2645

Testing

Added 2 test cases

Checklist

  • [x] I have read the contribution guidelines
  • [x] I have targeted this PR against the correct branch (master for website changes, dev for source changes)
  • [x] This is either a bugfix, a documentation update, or a new feature that has been explicitly approved via an issue
  • [x] I ran the test suite locally (npm run test) and verified that it succeeded

Telroshan avatar Sep 12 '24 16:09 Telroshan

After discussion with @1cg, here's the direction this PR should take:

  • add a syntax support for enclosing comma-selectors with extended ones, in the format closest <input, p/> (cf normalizeSelector)
  • keep global as a "apply to all" first-position-only keyword, and don't handle it in the loop at all (so, preserve current behavior, but don't introduce any new, we'll see if/when people using web components request such a feature, how we should implement it)

I'll work on these changes when I have the chance

Telroshan avatar Sep 25 '24 19:09 Telroshan

You might be able to add support for the < /> enclosed extended selectors with a change like this:

    let selectors = selector
    forEach(selectors.match(/<.*?\/>/g), function(enclosed) {
      selectors = selectors.replace(enclosed, enclosed.replaceAll(',', '%2C'))
    })
    const parts = selectors.split(',')

Trying to split on the commas but ignore the commas in the enclosed tags is very complex to do with just a regex so here I've just used the most basic regex to find all enclosed portions of the full selectors input string and then for each one replace all the commas with an encoded comma and update this in the main selectors string. Finally you can just add the reverse of this comma encoding in the normalizeSelector() function

https://github.com/bigskysoftware/htmx/commit/cf10b71bb0e0bd4cc47a4ef9ca23d501087b0fce example commit

One thing to note is the next/previous/closest/find extended selectors only return a single node by design so supporting multiple css selectors via the list , combinator will only return the first item found that matches one of the selectors that comes first. But they could now also do next input, next div instead of next <input, div/> if they wanted the next input AND div instead of next input OR div.

MichaelWest22 avatar Sep 30 '24 11:09 MichaelWest22

I'm fine w/ a loop-based mini-lexer, the logic isn't too brutal (basically count "<" and "/>") and ignore commas when count > 0) but I'm happy with whatever solution y'all come up w/

1cg avatar Sep 30 '24 17:09 1cg

Looks good @Telroshan!

was wondering how efficient this new version was compared to my more hacky solution i posted earlier https://github.com/bigskysoftware/htmx/commit/cf10b71bb0e0bd4cc47a4ef9ca23d501087b0fce and checking just the , parsing code on measurethat.net i got this:

Checked test: my-version x 774,172 ops/sec ±7.65% (48 runs sampled) Checked test: telroshan-version x 676,418 ops/sec ±3.14% (55 runs sampled) Checked test: telroshen-imporoved x 3,241,695 ops/sec ±7.49% (59 runs sampled)

So your version is about the same speed just slightly behind mine but then i found that 90% of the performance is lost in the single line:

} else if (selector.substring(i, i + 2) === '/>') {

updating this to

} else if (char === '/' && selector[i+1] === '>') {

got me to 3.2million ops/sec

MichaelWest22 avatar Nov 17 '24 23:11 MichaelWest22

Damn, that's an interesting benchmark, thanks for looking into it @MichaelWest22 ! I wouldn't have expected to be such a difference compared to using substring Just added an extra length check to avoid outs-of-bounds array access

Telroshan avatar Nov 18 '24 08:11 Telroshan

fun fact. unlike most languages there is no such thing as out of bounds array access error on strings/arrays in javascript. strings/arrays get auto extended with undefined if you read out of bounds so such checks are never needed if you already deal with the undefined case as === '>' already does for us here.

you could also do

        } else if (char === '>' && selector[i-1] === '/') {

or if you have to keep the out of bounds check for some reason:

        } else if (char === '>' && i > 0 && selector[i-1] === '/') {

MichaelWest22 avatar Nov 18 '24 09:11 MichaelWest22

@Telroshan can you update this PR against dev and resolve the (hopefully small) conflict?

1cg avatar Dec 11 '24 21:12 1cg

Sure, done @1cg Pushed a npm run format fix along, as otherwise the CI fails on the linting step

Telroshan avatar Dec 12 '24 08:12 Telroshan