-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
#652@patch: Allow deletion of nonexistent keys from dataset #1038
#652@patch: Allow deletion of nonexistent keys from dataset #1038
Conversation
This matches the behavior in the browser.
!!element.attributes.removeNamedItem('data-' + Dataset.camelCaseToKebab(key)) && | ||
delete dataset[key] | ||
); | ||
if (element.attributes.removeNamedItem('data-' + Dataset.camelCaseToKebab(key))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeNamedItem
might throw an NotFoundError, maybe it should be wrapped with a try catch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@btea Just to be clear, you're talking about making this change for future compatibility with the spec, correct? Because currently, the Happy DOM implementation of removeNamedItem
does not appear to ever throw that error. Please correct me if I'm wrong!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as you said, the current implementation of Happy DOM will return null
if the attribute does not exist.
So your current modification should be fine, maybe you can create a PR to modify the implementation compatibility specification of removeNamedItem
in the future.
According to the standard, if the attribute does not exist, an error will be thrown directly. There should be no need to wrap it with try catch
here. I thought wrong before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to make that change in this PR if you'd like! I can make removeNamedItem
throw an error if the item does not exist, and then make deleteProperty
catch the error. Does that sound correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct, but deleteProperty
should not need to catch errors, just throw them globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that deleting an item from a dataset should never throw an error, even if an item with that key does not exist.
const el = document.createElement("div")
delete el.dataset.blah // returns true; no error thrown
So I think you were right originally and we should catch the error thrown by removeNamedItem
in the deleteProperty
method of the dataset proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I confused removeNamedItem
and deleteProperty
, you are right. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll work on this tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@btea I just pushed those changes as discussed. Do you think this should still be considered a patch change? It technically might break someone's code if they were relying on removeNamedItem
not ever throwing, but that behavior was also technically incorrect, so I'm not sure whether to consider it a breaking change. My own feeling is it should still be a patch, but I wanted to check with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should still be a patch change.
53778b9
to
048fc3e
Compare
Thank you for contribution @RussianCow! ⭐ I would like to look into a solution where we don't have a use try/catch at that many places as it may have a significant performance impact. Hopefully I have some time over tomorrow to look into this. |
Thanks @capricorn86! That seems reasonable. We could have a separate Edit: Actually, it looks like Edit 2: Ahh, I see that |
When we want to call it, we will need to cast It is not pretty, but I guess it will work. |
Add a new method, `_removeNamedItem`, which removes the item without throwing if it does not exist, and override that in subclasses instead of the primary `removeNamedItem` method.
@capricorn86 Sorry for the delay, but I've made the changes discussed above. Let me know what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 🌟
Fixes #652. This matches the behavior in the browser.