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 multiple extended selectors for hx-include, hx-trigger from, and hx-disabled-elt #2902

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

Telroshan
Copy link
Collaborator

@Telroshan Telroshan commented Sep 12, 2024

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 issues: #2645 #2660 #2610

Testing

Added 2 test cases

Checklist

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

@Telroshan Telroshan added enhancement New feature or request ready for review Issues that are ready to be considered for merging labels Sep 12, 2024
src/htmx.js Outdated Show resolved Hide resolved
src/htmx.js Outdated Show resolved Hide resolved
src/htmx.js Outdated Show resolved Hide resolved
@Telroshan Telroshan removed the ready for review Issues that are ready to be considered for merging label Sep 20, 2024
@Telroshan Telroshan changed the title Support multiple extended selectors for hx-include Support multiple extended selectors for hx-include, hx-trigger from, and hx-disabled-elt Sep 22, 2024
@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Sep 23, 2024
src/htmx.js Outdated Show resolved Hide resolved
@Telroshan
Copy link
Collaborator Author

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 Telroshan removed the ready for review Issues that are ready to be considered for merging label Sep 25, 2024
@Telroshan Telroshan force-pushed the multiple-extended-selectors branch from 8e23149 to 4466697 Compare September 25, 2024 19:10
@MichaelWest22
Copy link
Contributor

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

cf10b71 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.

@1cg
Copy link
Contributor

1cg commented Sep 30, 2024

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/

@Telroshan Telroshan force-pushed the multiple-extended-selectors branch 2 times, most recently from 31dd615 to 48b2ec2 Compare November 17, 2024 22:24
@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Nov 17, 2024
@MichaelWest22
Copy link
Contributor

Looks good @Telroshan!

was wondering how efficient this new version was compared to my more hacky solution i posted earlier cf10b71 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

@Telroshan
Copy link
Collaborator Author

Telroshan commented Nov 18, 2024

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

@MichaelWest22
Copy link
Contributor

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] === '/') {

@1cg
Copy link
Contributor

1cg commented Dec 11, 2024

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

Telroshan and others added 4 commits December 12, 2024 09:00
Support multiple extended selectors for hx-include

Additional test for nested standard selector

Add @MichaelWest22 hx-disabled-elt multiple selector test

Add hx-trigger `from` test with multiple extended selectors

Simplify

Include bigskysoftware#2915 fix

Update htmx.js

Split for readability

Don't apply global to previous selectors

Rewrite loop, restore global recursive call, minimize diff

Use break for better readability

Co-Authored-By: MichaelWest22 <[email protected]>
@Telroshan Telroshan force-pushed the multiple-extended-selectors branch from 1e00251 to c3b1c59 Compare December 12, 2024 08:01
@Telroshan
Copy link
Collaborator Author

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

@1cg 1cg merged commit a331244 into bigskysoftware:dev Dec 12, 2024
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 ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants