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

Unable to disable inherit for hx-ext with hx-disinherit or htmx.config.disableInheritance #3016

Open
xhaggi opened this issue Nov 12, 2024 · 10 comments

Comments

@xhaggi
Copy link
Contributor

xhaggi commented Nov 12, 2024

I wonder why the inheritance of hx-ext is not done in the same way as the inheritance of all other attributes in htmx? If you disable the inheritance globally, this has no effect on hx-ext. Instead, you have to write the following to "ignore" the extension in a subtree, which is a bit annoying.

<form action="/user.html" method="post" hx-ext="my-form-extension">
  <div hx-ext="ignore:my-form-extension">
    <a hx-get="/details.html">Details</a>
  </div>
</form>

IMO it would be better to do the following:

<meta name="htmx-config" content='{"disableInheritance":"true"}'>

<form action="/user.html" method="post" hx-ext="my-form-extension">
    <a hx-get="/details.html">Details</a>
</form>

or

<form action="/user.html" method="post" hx-ext="my-form-extension" hx-disinherit="hx-ext">
    <a hx-get="/details.html">Details</a>
</form>
@MichaelWest22
Copy link
Contributor

Yeah inheritance in htmx can be a bit confusing and could do with improvements in documentation. There are three unique kinds of inheritance that I can find inside htmx.

There is attribute inheritance which applies to a subset of the common htmx attributes where inheritance would be useful like hx-target, hx-swap, hx-select etc. Attribute inheritance is the feature that is currently turned off with disableInheritance config.

There is also a special merge-key-inheritance used for attributes hx-vars, hx-vals, hx-headers and hx-request. These attributes do not work like the normal attributes as they apply a dynamic list of keys/values to all their children elements where it allows you to set multiple keys in multiple parent elements and the values are all merged together with most specific key overriding. This type does not respect the attribute inheritance disable config setting or the hx-inherit/hx-disinherit attributes as this does not fit the merge-keys system well and instead you have to use the not well documented hx-vals="unset" option on a child element to disable inheritance. see #2847

And finally we have hx-ext inheritance which is currently implemented with its own style with "ignore:XXX" as the only way to remove inheritance currently. I don't think we can reuse the attribute inheritance based disableInheritance config as this would be a breaking change that would impact any deployed sites that use this config to disable attribute inheritance but also want to use extensions. Many of the extensions work best by placing the hx-ext on the body element as they make very global changes to how htmx works and moving these to apply only to the one element would be a big change in expected behavior.

hx-ext just like how the merge-key-inheritance doesn't work well with the hx-inherit/hx-disinherit pattern. The hx-ext is actually very similar where each hx-ext attribute can set a list of extensions that apply to this element and all children like hx-ext="ext1,ext2,ext3". Which is why I'm guessing the hx-ext="ignore:ext2" option was implemented so that you have fine grained control to remove just the extensions you need to disable but leave the other global extensions.

In theory hx-disinherit attribute could be overloaded with new code to enable disinheritance for hx-ext like in your last example but it does add some more complexity to the htmx source to do this when there is already a documented way of doing it. Doing this would have a small performance impact as well as scanning for extension attributes happens many many times for every request and now it has to scan twice as many attributes every time. You could also make it work the same for the merge-key-inheritance attributes case as well to make it more universal. It makes no sense to do hx-inherit here though. Another simpler option could be to implement and document better the "unset" pattern used for merge-key-inheritance as this would be a much simpler code change with no performance impact where you do:

<form action="/user.html" method="post" hx-ext="my-form-extension">
     <a hx-get="/details.html" hx-ext="unset">Details</a>
</form>

Which disables ALL extensions on the element and its children.

Also right now you can simplify your initial example right now down to:

<form action="/user.html" method="post" hx-ext="my-form-extension">
     <a hx-get="/details.html" hx-ext="ignore:my-form-extension">Details</a>
</form>

@MichaelWest22
Copy link
Contributor

Examples of how you could add one if check for 'unset' and return early so you can stop all extension inheritance. needs 3 lines of code:

  function getExtensions(elt, extensionsToReturn, extensionsToIgnore) {
    if (extensionsToReturn == undefined) {
      extensionsToReturn = []
    }
    if (elt == undefined) {
      return extensionsToReturn
    }
    if (extensionsToIgnore == undefined) {
      extensionsToIgnore = []
    }
    const extensionsForElement = getAttributeValue(elt, 'hx-ext')
    if (extensionsForElement == 'unset') { //  <--------- Just this one if statement added to return early when it finds unset so it won't continue search up the parent tree from here.  you can't set any extensions on the same element you use unset on.
      return extensionsToReturn
    }
    if (extensionsForElement) {
      forEach(extensionsForElement.split(','), function(extensionName) {
        extensionName = extensionName.replace(/ /g, '')
        if (extensionName.slice(0, 7) == 'ignore:') {
          extensionsToIgnore.push(extensionName.slice(7))
          return
        }
        if (extensionsToIgnore.indexOf(extensionName) < 0) {
          const extension = extensions[extensionName]
          if (extension && extensionsToReturn.indexOf(extension) < 0) {
            extensionsToReturn.push(extension)
          }
        }
      })
    }
    return getExtensions(asElement(parentElt(elt)), extensionsToReturn, extensionsToIgnore)
  }

Examples of how to allow hx-ext="my-custom-ext,unset" so you could also enable one or more extensions while disabling all parent inheritance at the same time. this takes 3 bits added and 8 lines of code:

  function getExtensions(elt, extensionsToReturn, extensionsToIgnore) {
    if (extensionsToReturn == undefined) {
      extensionsToReturn = []
    }
    if (elt == undefined) {
      return extensionsToReturn
    }
    if (extensionsToIgnore == undefined) {
      extensionsToIgnore = []
    }
    const extensionsForElement = getAttributeValue(elt, 'hx-ext')
    let unset = false  // define unset boolean to false initially
    if (extensionsForElement) {
      forEach(extensionsForElement.split(','), function(extensionName) {
        if (extensionName == 'unset) {  // set unset variable true if found
          unset = true
          return
        }
        extensionName = extensionName.replace(/ /g, '')
        if (extensionName.slice(0, 7) == 'ignore:') {
          extensionsToIgnore.push(extensionName.slice(7))
          return
        }
        if (extensionsToIgnore.indexOf(extensionName) < 0) {
          const extension = extensions[extensionName]
          if (extension && extensionsToReturn.indexOf(extension) < 0) {
            extensionsToReturn.push(extension)
          }
        }
      })
    }
    if(unset) {  // add a return here if any unset found to stop it scanning parents
      return extensionsToReturn
    }
    return getExtensions(asElement(parentElt(elt)), extensionsToReturn, extensionsToIgnore)
  }

And finally an option to have a hx-ext="local:my-custom-ext" option to apply an extension to the local element only without applying it to children via inheritance or having to ignore: on all children. requires 3 additions and 9 more lines of code

  function getExtensions(elt, extensionsToReturn, extensionsToIgnore) {
    let parentScan = true  // add a variable for if we are scanning parents
    if (extensionsToReturn == undefined) {
      parentScan = false  // set the parent scan false on first call where extensionsToReturn are undefined
      extensionsToReturn = []
    }
    if (elt == undefined) {
      return extensionsToReturn
    }
    if (extensionsToIgnore == undefined) {
      extensionsToIgnore = []
    }
    const extensionsForElement = getAttributeValue(elt, 'hx-ext')
    if (extensionsForElement) {
      forEach(extensionsForElement.split(','), function(extensionName) {
        extensionName = extensionName.replace(/ /g, '')
        if (extensionName.slice(0, 7) == 'ignore:') {
          extensionsToIgnore.push(extensionName.slice(7))
          return
        }
        if (extensionName.slice(0, 6) == 'local:') {  // for local: prefix return if its a recursive parent scan to ignore this extensions inheritance and otherwise strip the local: prefix
          if (parentScan) {
            return
          }
          extensionName = extensionName.slice(6)
        }
        if (extensionsToIgnore.indexOf(extensionName) < 0) {
          const extension = extensions[extensionName]
          if (extension && extensionsToReturn.indexOf(extension) < 0) {
            extensionsToReturn.push(extension)
          }
        }
      })
    }
    return getExtensions(asElement(parentElt(elt)), extensionsToReturn, extensionsToIgnore)
  }

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 13, 2024

Thank you for the detailed answer. For me, it would be good if there is a way to disable inheritance globally. BTW, I don't like the idea of using unset to disable inheritance because it also needs a subtree element to configure it. How about introducing a new attribute hx-ext-disinherit="my-ext,my-other-ext" or hx-ext-disinherit="*" and the configuration setting htmx.config.disableExtentionInheritance (default: false). This way it is more aligned with the current inheritance implementation and does not introduce any breaking changes.

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 13, 2024

@Telroshan mind taking a look?

@Telroshan
Copy link
Collaborator

TL;DR

  • I like the local: proposal
  • I like the htmx.config.disableExtensionInheritance proposal
  • Not a fan of the other suggestions

Wall of text

For me, it would be good if there is a way to disable inheritance globally

I agree with that

As Michael pointed out, we have the existing system of hx-ext="ignore:" to turn off an extension for an element and all its descendants.
The issue I see with that current implementation is that you would have to define ignore on every direct child to make sure the extension doesn't apply to any descendant, which isn't ideal.

If I make up an example, I can imagine someone wanting to enable say json-enc on a form, but not on any standalone <button type="button" hx-get="..."> or other hx-powered element within, and there is currently no simple way to say "enable json-enc just for this very element"

  • hx-ext="unset" (or hx-ext="ignore:*" to mimic the syntax of hx-inherit/hx-disinherit) suffers the same issue in my opinion than the usecase I mentioned before ; I imagine the user will more likely want to say "enable this extension just for this element" rather than "cut the parent inheritance chain starting from this elt". The issue with that syntax is that the element you want to enable the extension for itself only, will need you to define hx-ext="unset" on any direct child that might have hx-powered descendants (as the element itself will have a hx-ext="whatever" defined, thus cannot define unset at the same time)
  • hx-ext="my-custom-ext,unset", while solving the usecase of "discard any inherited extension and enable only this one for me", wouldn't unfortunately solve the "enable the extension just for me and no descendant" one. I guess we could play with the order of declaration, like unset, extension would drop inherited extensions and enable extension on the elt + children, while extension, unset would enable extension on the element itself, and disable inheritance for any descendant when getExtension called on a descendant would scan the DOM upwards (then you could also do hx-ext="unset, extension, unset"). Doesn't sound very intuitive to me though
  • hx-ext="local:my-custom-ext" sounds like a great idea to me. Making up an example again (though one that applies to myself so not that much made up), I could want to enable hx-sse on the body to have multiple descending elements to use SSE behavior, and from time to time enable another extension for a specific element (let's say json-enc on a specific form again) but not its descendants, while keeping extensions inheritance enabled by default (as most of the extensions we have make sense to put on the body tag and enable for the whole DOM for ex).
    I like to have explicit inheritance for other attributes (I really don't like to have hx-target be inherited in my projects for ex), but for extensions, it makes much more sense imo. Adding an explicit non-inheritance here sounds good to me. Also, since we already have the ignore: syntax (explicit disinheritance), I'd say the local: syntax makes sense and is in line.
  • htmx.config.disableExtensionInheritance sounds like a great idea to me, as you can add a simple boolean check, and prevent the recursive ascending calls of getExtensions if it's true. It perfectly makes sense to me to say "I just don't want any extensions to be inherited by default in my project"
  • I'm a bit more skeptical about hx-ext-disinherit though, I'll try to explain why ; I would imagine the usecases to be to enable a specific extension for a specific element, to not want inheritance at all by default, or to have inheritance by default.
    I'm having a hard time imagining a usecase where you would want to enable an extension on an element and all its descendants, but on a specific descendant, disable this extension, not just for this element but for any of its descendants as well. If you have an example usecase in mind, of course prove me wrong!
    The usecases I can think of are:
    • Let extensions be inherited everywhere by default : the default, current state
    • Prevent extensions from being inherited at all by default, to define them explicity everywhere needed : the htmx.config.disableExtensionInheritance proposal solves this, and you would simply need to add hx-ext to any element that needs any extension
    • Enable an extension just for a specific element and no descendants, while keeping inheritance enabled by default : the local: proposal solves this (for ex, SSE enabled on the body for the entire document, but enable json-enc on a specific form only)
    • Disable a specific extension on an element and its descendants, while keeping inheritance enabled by default elsewhere : the ignore: syntax of hx-ext solves this, as the recursive processing will have the element and any of its descendants ignore the extension (similar to what hx-ext-disinherit would achieve then)

So, to me, local: solves the relevant part of what hx-ext-disinherit would allow, while preventing an extra attribute and staying in line with the existing syntax, and the part it doesn't cover is a convoluted usecase anyway that you probably want to achieve some other way

@MichaelWest22
Copy link
Contributor

Yeah the local: idea does seem quite a good option for the edge cases where an extension causes issues for sub elements. I'm not a big fan myself of the htmx.config.disableExtensionInheritance option but it's more just my personal preference. In my mind having hx-ext always default to inherited just makes more sense to me as if you have an extension called 'my-form-extension' applied to a form tag you would expect that the whole form would get the new functionality and not just the form wrapper element. Most of the time with extensions you apply it at a given DOM level and then on various elements under that level you add custom extension specific tags to target and apply the extension behavior as required or you expect it to just modify htmx behavior for that area of the page. like most of the time if you have a my-form-extension applied to a form other elements inside the form that are not forms themselves will not be impacted by the form extension anyway so why do we need to complicate it by trying it disable the scope of the extension? There are going to be exceptions of course like the json-enc example where you will find it could cause unexpected issues and need a ignore: or local: change to target the scope of the extension.

Just Did a quick PR with the local: idea to try and see how the change looks when done properly. Also optimized readability and performance a little to counter the added complexity being added. I can add disableExtensionInheritance config option if there is a real need for this as well.

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 14, 2024

Most of our extensions are element specific and should not be inherited by default. I'm not a big fan of having different approaches for the same functionality. For attribute inheritance you have to use hx-disinherit, but for extensions you have to use this weird prefix style. BTW, with hx-disinerit it should also be possible to disable the entire inheritance of hx-ext, since it is an attribute, while hx-ext-disinherit can be used for specific extensions. The prefix ignore can be retained for backward compatibility.

@Telroshan
Copy link
Collaborator

  • For attribute inheritance you have to use hx-disinherit, but for extensions you have to use this weird prefix style

    On a broader aspect, attributes inheritance lets you carry values down if you don't explicitly define them on children. Declaring an attribute will override thus ignore the parent value. For extensions, the core principle is totally different ; a child declaration will append extensions to the parent ones, so the context is not the exact same.
    As Michael pointed out, extensions work much more like hx-vals/hx-vars in this case, which also work with a merging logic. So, about the consistency/different approaches for the same functionality, I think it makes more sense to compare extensions to hx-vals/var rather than standard attributes.
    Yet again, they differ ; hx-vals/vars are used to provide additional values, while extensions enable additional behaviors. So hx-ext is likely halfway between standard attributes and hx-vars/vals.

  • The prefix ignore can be retained for backward compatibility.

    Even with hx-ext-disinherit added to the core, ignore: still serves a purpose, and would not just be for backwards compatibility imo.
    You could want an element to let its enabled extension be inherited, but disable it on a very specific child.
    The way hx-disinherit works is that it lets you define, on the parent, which attributes you don't want to let be inherited, but in this case you need a way to disable a specific extension on the child itself. With standard attributes, it's easy ; you just define that attribute on the child with the new value (or unset for some of them to "null them out").
    Though, for extensions, it wouldn't work since these child-defined extensions would be appended, and not override the parent ones.
    Specifying hx-ext-disinherit on the parent would prevent the child's siblings from getting the extension, which isn't what we would want here, in which case ignore would still be useful, and provide a feature that hx-ext-disinherit would not.
    Note that again, this is due to the fact that extensions inheritance doesn't work like standard attributes, for which you could simply override the parent declaration on the child, while extensions append to the parent ones.

Given that the ignore: prefix would still be useful, it's then probably a matter of opinion to either add self-only-extension be in line with the existing ignore syntax, or be in line with the standard attributes inheritance.
I now see the value of hx-ext-disinherit, which would make things more consistent with standard inheritance, but it would only do so partly. You would have some of the behavior in hx-ext-disinherit, an extra attribute, and some other retained in hx-ext itself with the ignore: prefix.
I also see the value of local:, which would keep those behaviors in hx-ext itself and not spread them in different attributes, but it would add yet another mini-syntax as you pointed out.

Probably a matter of taste to decide between those 2, but I don't think any of them perfectly solves the consistency-with-other-features issue. I personally like the local: prefix better as it keeps things within the hx-ext attribute itself

  • BTW, with hx-disinherit it should also be possible to disable the entire inheritance of hx-ext

    Yup, that makes perfect sense to me, regardless of its implementation and what we decide for the topic discussed above, hx-ext remains an attribute and should be explicitly disinheritable, as well as the counterpart explicit hx-inherit if extensions inheritance is disabled via global config. This would probably not require huge changes in the code to be supported, and would work well along the htmx.config.disableExtensionInheritance suggesstion imo.
    To ease the PR reviews btw and ease 1cg's work, I would recommend making a separate PR from the local: one, that would add htmx.config.disableExtensionInheritance + hx-inherit/hx-disinherit support for hx-ext

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 29, 2024

What are the next steps here?

@Telroshan
Copy link
Collaborator

@xhaggi hopefully 1cg will have some time available to review pending PRs & cut a new release by the end of the year.

While Michael's PR is pending review with its local: change, if you're interested in it, feel free to open another PR with the htmx.config.disableExtensionInheritance + hx-inherit/hx-disinherit support for hx-ext changes we talked about
Can't guarantee that 1cg will have the same opinion as me, so if you want to advocate for the suggestions I personally didn't like, there's still a chance for them to make it through as it's not my sole opinion that determines what gets merged in the end 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants