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

[bug] Handle missing selectors for closest/find/next/previous #2915

Closed

Conversation

MichaelWest22
Copy link
Contributor

@MichaelWest22 MichaelWest22 commented Sep 17, 2024

Description

when using some of the custom selector modes like find etc it now wraps the response in an array. But when there happens to be no elements found it then wraps the null which will throw exceptions up the chain so we need to get it to return empty array instead.

Corresponding issue:
#2913

Testing

Added test and did manual testing to confirm find etc does not throw errors anymore when no elements are found

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 the bug Something isn't working label Sep 18, 2024
Copy link
Collaborator

@Telroshan Telroshan left a comment

Choose a reason for hiding this comment

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

Nice, simple & effective fix!

@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Sep 18, 2024
@MichaelWest22
Copy link
Contributor Author

If we merge in #2902 first then this PR would need to be updated to use result.push(...toNodeArray(xxxx))

@MichaelWest22
Copy link
Contributor Author

actually to merge this fix into #2902 you can simplify it down to this:

let item
      if (selector.indexOf('closest ') === 0) {
        item = closest(asElement(elt), normalizeSelector(selector.substr(8)))
      } else if (selector.indexOf('find ') === 0) {
        item = find(asParentNode(elt), normalizeSelector(selector.substr(5)))
      } else if (selector === 'next' || selector === 'nextElementSibling') {
        item = asElement(elt).nextElementSibling
      } else if (selector.indexOf('next ') === 0) {
        item = scanForwardQuery(elt, normalizeSelector(selector.substr(5)), !!global)
      } else if (selector === 'previous' || selector === 'previousElementSibling') {
        item = asElement(elt).previousElementSibling
      } else if (selector.indexOf('previous ') === 0) {
        item = scanBackwardsQuery(elt, normalizeSelector(selector.substr(9)), !!global)
      } else if (selector === 'document') {
        item = document
      } else if (selector === 'window') {
        item = window
      } else if (selector === 'body') {
        item = document.body
      } else if (selector === 'root') {
        item = getRootNode(elt, !!global)
      } else {
        // Push back the unaltered string part, so that the standard selector fallback uses the exact initial string
        unprocessedParts.push(selector)
      }
      item && result.push(item)

Where you change it from pushing to the array each time to assign to a let variable and then only push at the end if item is not null or undefined which handles the null case well.

@Telroshan
Copy link
Collaborator

Yep makes sense @MichaelWest22
Given that #2902 already generates a diff on all lines of that function, I think I'll directly embed your fix in here as I don't expect it to make reviewing any harder

Telroshan added a commit to Telroshan/htmx that referenced this pull request Sep 19, 2024
Co-Authored-By: MichaelWest22 <[email protected]>
@Telroshan
Copy link
Collaborator

Merged your changes in #2902, closing this one!

@Telroshan Telroshan closed this Sep 19, 2024
@Telroshan Telroshan removed the ready for review Issues that are ready to be considered for merging label Sep 19, 2024
Telroshan added a commit to Telroshan/htmx that referenced this pull request Sep 25, 2024
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 added a commit to Telroshan/htmx that referenced this pull request Nov 17, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants