#652@patch: Allow deletion of nonexistent keys from dataset

[RussianCow]

Fixes #652. This matches the behavior in the browser.

[btea]

removeNamedItem might throw an NotFoundError, maybe it should be wrapped with a try catch.

[RussianCow]

@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!

[btea]

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.

[RussianCow]

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?

[btea]

I think this is correct, but deleteProperty should not need to catch errors, just throw them globally.

[RussianCow]

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.

[btea]

Sorry, I confused removeNamedItem and deleteProperty, you are right. 👍

[RussianCow]

I'll work on this tomorrow!

[RussianCow]

@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.

[btea]

I think this should still be a patch change.

[capricorn86]

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.

[RussianCow]

Thanks @capricorn86! That seems reasonable. We could have a separate _removeNamedItemWithoutThrowing method that simply returns a bool indicating whether the item existed, and have Dataset and Element use that directly instead? Then removeNamedItem can just check the bool and throw if it's false. What do you think? I can throw this together later today.

Edit: Actually, it looks like _removeNamedItemWithoutConsequences already exists. We could just call that directly from the other classes?

Edit 2: Ahh, I see that _removeNamedItemWithoutConsequences is not exposed in INamedNodeMap, so that likely won't work. I'm curious to see how you'd like to handle this. 😃

[capricorn86]

Thanks @capricorn86! That seems reasonable. We could have a separate _removeNamedItemWithoutThrowing method that simply returns a bool indicating whether the item existed, and have Dataset and Element use that directly instead? Then removeNamedItem can just check the bool and throw if it's false. What do you think? I can throw this together later today.

Edit: Actually, it looks like _removeNamedItemWithoutConsequences already exists. We could just call that directly from the other classes?

Edit 2: Ahh, I see that _removeNamedItemWithoutConsequences is not exposed in INamedNodeMap, so that likely won't work. I'm curious to see how you'd like to handle this. 😃

_removeNamedItemWithoutConsequences() is not exactly the same, as it exists to be able to remove an item without consequences, such as removing the "style" attribute will empty the "style" property from its CSS etc. We still want those consequences to happen. I guess we could have a new method called _removeNamedItem() that just returns the removed attribute or null. Then all sub classes needs to override that method instead of removeNamedItem().

When we want to call it, we will need to cast INamedNodeMap to NamedNodeMap. E.g. (<HTMLElementNamedNodeMap>this.attributes)._removeNamedItem().

It is not pretty, but I guess it will work.

[RussianCow]

@capricorn86 Sorry for the delay, but I've made the changes discussed above. Let me know what you think!

[capricorn86]

Nice! 🌟