fix: use proxy for attributes getter (#728)

[lukasbach]

This fixes the issue described in #728.

[lukasbach]

Workaround for this: https://stackoverflow.com/a/75153646

[jaa134]

I'm curious where this implementation came from? It seems like you are referencing values from this._attributes instead of target arbitrarily, but I'm sure I am missing something.

[lukasbach]

See #876 (comment)

[jaa134]

I'm not sure this solves the problem. While you may be accessing updated values from this._attributes, your NamedNodeMap is never actually populated with the attributes. This is problematic because interfacing with the NamedNodeMap directly using any of its instance properties or methods will effectively function on an empty map.

Therefore:

console.log(el.attributes.length) // will always return 0
el.attributes.setNamedItem // will never have any effect
el.attributes.removeNamedItem // will never have any effect

In practice, calling any of the above instance methods on an element in a real browser will have side effects on the element itself i.e. calling el.attributes.removeNamedItem('class') will remove the class attribute on element el in the dom. But this is not true for the proposed solution or for the current implementation in happy-dom.

I think a better long term solution is to retype this._attributes to a NamedNodeMap and to have this attributes accessor simply return this._attributes.

[lukasbach]

Hey, thanks for taking your time to review. I'll try to explain my way of thinking in one comment, I think that is easier.

So my primary goal was to fix the linked issue, which prevents happy-dom to be used with anything that uses microsoft-fast, which uses element attributes in a certain way, specifically by reading the attributes object and reading from it multiple time subsequently. I agree that a better long-term solution would be to implement a more comprehensive mock of a named node map that supports array access, but I'm not sure if I currently have the capacity for that at the moment. So instead, my scope was to improve what exists at the moment.

I'm curious where this implementation came from? It seems like you are referencing values from this._attributes instead of target arbitrarily, but I'm sure I am missing something.

The intend behind the node.attributes instance is: return an instance of a named node map (which is available as class in happy-dom, thus previously mocked with new NamedNodeMap(this)), but also allow named access for attributes like attributes.attributeName (previously mocked by asssigning this._attributes to the namedNodeMap) and also array-style access like attributes[0] (previously mocked by assigning Object.values(this._attributes)).

My proposed logic is: We proxy all getter accesses on the named node map, and

• if it is a number, the consumer is doing a array-style access (attributes[0]), so we do Object.values(this._attributes)[parseInt(name, 10)] (if you're wondering why it's still a string, see next section)
• if it is a string and exists as named attribute in the attributes map, it is a named access like attributes.attributeName, so we return the value from the attributes map
• otherwise, we let the original named node map (target) handle the get

The first if-block is a workaround for function calls on the named node map: proxying getters on function attributes leads to the issue described in https://stackoverflow.com/a/75153646, so we wrap the function which works as a workaround. I could have added that check at the end as well, but readability and type-checking is a bit easier by moving that check to the top.

The second if-block (if typeof name is symbol) is mostly to make typescript happy. I think there is no legal way to make a get on a named node map with a symbol as name, but the type of a proxy getter allows a symbol as valid name, so I let the named node map (target) handle it, which I suppose was the old behaviour as well.

What if the passed in value is of type number? I would expect the value returned to be pulled from Object.values(this._attributes), but the current flow would have it return from line 272.

I'm not sure why, but the name attribute the proxy getter receives can only either be a string or a symbol. Calling node.attributes[123] ends in name being the string "123" so JS for some reason maps that to a string somewhere internally. That's why the regex and parseInt is there.

While you may be accessing updated values from this._attributes, your NamedNodeMap is never actually populated with the attributes.

I think it is, or at least to the same extend as before? Previously, node.attributes every time returned a new instance of NamedNodeMap(this) which handled method accesses. The proxy also proxies a new instance of NamedNodeMap(this) and proxies all method calls to this instance, so the effect of calling methods on the map should be the same as before.

I think a better long term solution is to retype this._attributes to a NamedNodeMap and to have this attributes  accessor simply return this._attributes.

Kinda agree, but also not sure how that could be done. We would still need to adjust the named node map implementation to support array-style and arbitrary key access then, and to be honest I'm not sure how to do that without a proxy.

I hope I didn't bore you too much with my text, but hope I could clear up my intent a bit 😅.

[jaa134]

I totally understand the concept of not having enough time. 😅 That resonates with me...

I think it is, or at least to the same extend as before? Previously, node.attributes every time returned a new instance of NamedNodeMap(this) which handled method accesses. The proxy also proxies a new instance of NamedNodeMap(this) and proxies all method calls to this instance, so the effect of calling methods on the map should be the same as before.

I guess my point is that yes, this might stop errors from surfacing when attributes are removed twice, but there is still a much larger issue that will probably lead to false test execution results. In its current and proposed implementations, modifying the instance of the NamedNodeMap that is returned from the attributes accessor will never modify the underlying _attributes property on the Element class. Consider the following example:

const el = document.querySelector('.my-class'); // find some element with a class attribute
const attributes = el.attributes; // a NamedNodeMap that contains a class attr entry
attributes.removeNamedItem('class'); // this SHOULD modify el's attributes to remove the class entry but it doesn't
console.log(el.attributes) // prints the attributes and incorrectly includes the class attr that should have been removed

As you can see, el.attributes likely to get out of sync with the underlying _attributes property.

You're right that it will work to the same extent as before because the NamedNodeMap wasn't stateful before this PR (which means that it has the same problem). So, maybe this PR is fine and a new one should be made to address the larger issue that I am describing.

Thinking about it more today, I think a fully-fledged solution might look like:

1. Change the type of _attributes to NamedNodeMap
2. Then the attributes accessor simply becomes public get attributes(): INamedNodeMap { return this._attributes }
3. And finally Element.{getAttributeNode,setAttributeNode,removeAttributeNode} are all delegated to the built-in methods on NamedNodeMap. The logic for these methods and other lesser methods would be moved to the NamedNodeMap file.

I'm going to let a maintainer of this repo decide what the next steps should be here. In the meantime, I will try to find time to open a PR with a fix.

[capricorn86]

Thank you for contributing @lukasbach! 🌟

I haven't had time to look into this in detail, but my spontaneous thought is that we need to find a solution where we update the Element.attributes property directly and remove the Element._attributes property, if we are going to be able to find a solution which doesn't hurt performance and still updates attributes live in the NameNodeMap.

[capricorn86]

@lukasbach sorry for taking such a long time to solve this. I have fixed it in #1006 now.