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

🐛 Prevents erroring on null vals #2799

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Aug 6, 2024

Description

This prevents accessing the forEach key on null or undefined values. null is valid in JSON, so it should be supported by the hx-vals attribute. This is a regression from 1.x

Could use optional chaining, but I'd imagine that isn't an accepted feature. I also assume the fact it doesn't use Array.isArray is because it wants to support other containers that implement forEach. So just doing a truthy test is satisfactory, since there is 0% chance that someone is going to be passing document.all as a value.

Corresponding issue:

Testing

Added test. Change is common sense, but the test ensures it's condition into the future.

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 bug Something isn't working 2.0 labels Aug 7, 2024
@Telroshan
Copy link
Collaborator

Hey, good catch!
You'll probably also want to add this check for formDataProxy's set method, where we don't check if value is truthy at all either?

htmx/src/htmx.js

Lines 4004 to 4015 in eb32358

set: function(target, name, value) {
if (typeof name !== 'string') {
return false
}
target.delete(name)
if (typeof value.forEach === 'function') {
value.forEach(function(v) { target.append(name, v) })
} else {
target.append(name, value)
}
return true
},

@ekwoka
Copy link
Contributor Author

ekwoka commented Aug 7, 2024

Implemented that. I couldn't see easily how to do a test as sufficiently high level for how the tests are set up of that.

@Telroshan
Copy link
Collaborator

@ekwoka the formDataProxy is used for example by getInputValues, which returns it in its values property.
getInputValues is itself exposed in the internal API and I seem to recall we have a bunch of tests for this function specifically, so I think you could add one here by using the proxy syntax to do something like result.values["test"] = null?
Didn't test this though, you would want to make sure first that this indeed throws an error without your fix

@ekwoka
Copy link
Contributor Author

ekwoka commented Aug 8, 2024

Yes, I will try to identify it. I just couldn't really tell where a proper test of that feature would be or what it would look like. Thanks for the feedback, I will try to identify that spot, validate the error, and add the test.

@ekwoka
Copy link
Contributor Author

ekwoka commented Aug 9, 2024

Okay! Got it! I removed the fix on the formdata proxy, validated that this test fails (from the throw), and that the fix works.

@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Aug 9, 2024
@ekwoka
Copy link
Contributor Author

ekwoka commented Aug 28, 2024

Any updates or concerns?

@1cg 1cg merged commit b23b2f0 into bigskysoftware:dev Oct 3, 2024
1 check passed
@1cg
Copy link
Contributor

1cg commented Oct 3, 2024

nope just me moving slow, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 bug Something isn't working 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