bigskysoftware / htmx

</> htmx - high power tools for HTML
https://htmx.org
Other
37.43k stars 1.26k forks source link

HTMX raises error when passing null value in hx-vals #2884

Open tarunbhardwaj opened 1 week ago

tarunbhardwaj commented 1 week ago

HTMX currently throws an error when a null value is passed in the hx-vals attribute due to improper handling of null values.

https://github.com/bigskysoftware/htmx/blob/326ff3b296cf5e3eb0ec7799a9f607818776628c/src/htmx.js#L3965

The issue arises because the existing logic checks for the forEach method without verifying if the object itself is non-null, leading to an exception when null values are encountered.

Steps to Reproduce:

  1. Create an element using hx-vals with a null value, for example:
    <div hx-post="/" hx-vals="javascript:{key1:null}"></div>
  2. Trigger the POST request (e.g., by clicking the element).
  3. Observe the error in the console due to improper handling of the null value.

Expected Behavior: HTMX should handle null values gracefully without raising an error, and the form data should be constructed correctly.

Proposed Fix:

  1. Update the condition in htmx.js to ensure the object is non-null before attempting to invoke the forEach method.
    • Example change:
      if (obj[key] && typeof obj[key].forEach === 'function') {
  2. Add a unit test to validate that null values in hx-vals do not cause errors and are correctly handled.

Related PR: https://github.com/bigskysoftware/htmx/pull/2883

MichaelWest22 commented 1 week ago

It seems right now if any of the form values calculated via hx-vals or other forms happens to be either null or undefined preventing all queries. So I think this is kind of a bug. The question is what should htmx do when someone passes in invalid null or undefined value for a property. having the form data say key1=undefined or key1=null useful for the backend to get? Or should we be instead just stripping and ignoring these parameters and only send the valid ones? One potential issue allowing "null" and "undefined" to make it though is that they come though to the backend as just strings and not true null or undefined values which could cause confusion. Also someone just raised an issue #2885 where they want to optionally not return a property if it is not set and having issues with the syntax to do this and allowing undefined to not define a parameter value could be a great solution to this.

For example could add obj[key] != null like:

      for (const key in obj) {
        if (obj.hasOwnProperty(key) && obj[key] != null) {
          if (typeof obj[key].forEach === 'function') {
            obj[key].forEach(function(v) { formData.append(key, v) })

And then both null and undefined are not sent as parameters

Another option is:

      for (const key in obj) {
        if (obj.hasOwnProperty(key) && obj[key] !== undefined) {
          if (obj[key] && typeof obj[key].forEach === 'function') {
            obj[key].forEach(function(v) { formData.append(key, v) })

Where key1:undefined is stripped but key1:null is passed fine so you can send the string null.

Because neither null or undefined work at all right now I don't think any of these options would be a breaking change.

shouya commented 1 week ago

I think it's probably not a good idea to skip any null fields. If we don't allow null values to be encoded in the request, we lose a state that's representable. In the case of GET requests, ?key= and an empty query can be distinguished. Similarly for POST, {"key":null} can have different semantics than {} depending on server-side interpretation.

But I do agree that undefined value can be discarded because the undefined is already semantically used to represent missing value. So it makes more sense this way.

tarunbhardwaj commented 1 week ago

What if undefined is ignored and null is changed to an empty string, like in HTML forms, and '' is used for a null value? What do you think?

MichaelWest22 commented 1 week ago
for (const key in obj) {
      if (obj.hasOwnProperty(key) && obj[key] !== undefined) {
        if (obj[key] && typeof obj[key].forEach === 'function') {
          obj[key].forEach(function(v) { formData.append(key, v) })
        } else if (typeof obj[key] === 'object' && !(obj[key] instanceof Blob)) {
          formData.append(key, JSON.stringify(obj[key]))
        } else {
          formData.append(key, obj[key] === null ? '' : obj[key])
        }
      }

To do that we would need to make a change to 3 lines like this.

tarunbhardwaj commented 1 week ago
    for (const key in obj) {
      if (obj.hasOwnProperty(key) && obj[key] !== undefined) {
        if (obj[key] == null) {
          formData.append(key, '')
        }
        else if (typeof obj[key].forEach === 'function') {
          obj[key].forEach(function(v) { formData.append(key, v) })
        } else if (typeof obj[key] === 'object' && !(obj[key] instanceof Blob)) {
          formData.append(key, JSON.stringify(obj[key]))
        } else {
          formData.append(key, obj[key])
        }
      }
    }

Is this readable?

Telroshan commented 1 week ago

Note: see #2799 that addresses this (pending review, should be in the next update)