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

Provide getAttributeNames() which returns an Array<DOMString> #115

Closed
esprehn opened this issue Nov 21, 2015 · 17 comments
Closed

Provide getAttributeNames() which returns an Array<DOMString> #115

esprehn opened this issue Nov 21, 2015 · 17 comments

Comments

@esprehn
Copy link

esprehn commented Nov 21, 2015

Element.prototype.attributes requires allocating all the Attr node instances and the NamedNodeMap itself which is expensive, lots of frameworks (ex. Polymer, Angular, others) just want to iterate the attributes at construction time of an element or when traversing the document and don't need these expensive persistent structures.

Instead we should provide getAttributeNames(), then authors can use that with getAttribute/setAttribute/removeAttribute which results in better memory usage and performance.

This would also get developers to stop using .attributes which means the usage of the exotic Node like thing would go down possibly allowing the removal of it in the future.

@domenic @foolip

@foolip
Copy link
Member

foolip commented Nov 21, 2015

That sounds pretty nice, not unlike Object.getOwnPropertyNames.

My biggest question mark is around namespaces, without which this would be a simple API. Because of them, one can't return an array of strings where each string points to a unique attribute:

var e = document.createElement('span');
e.setAttributeNS('http://a.example.com/', 'foo', 'bar');
e.setAttributeNS('http://b.example.com/', 'foo', 'baz');
alert(e.getAttribute('foo'));
e.removeAttribute('foo');
alert(e.getAttribute('foo'));

I'm sure you could come up with a crazy case involving prefixes too.

Should the API throw an exception if any of the attributes are in a namespace, or return a tuple of strings instead of a string for those cases?

@annevk
Copy link
Member

annevk commented Nov 24, 2015

I think we should just return the qualified names, those are the property names NamedNodeMap already exposes and what most algorithms operate on.

If you just stick to namespaceless attributes as you should, there will be no issue.

@foolip
Copy link
Member

foolip commented Nov 24, 2015

With qualified names you could still have the same string appear multiple times, do we just live with that? It would be nice if one didn't have to resort to NamedNodeMap to guarantee that all attributes are found, but I see no other way to give that guarantee than to have a namespace+localName tuple :(

@annevk
Copy link
Member

annevk commented Nov 24, 2015

Yeah, and to be fair, for mutation observers we do care about all attributes. Perhaps we should return an array of two-item-arrays ([namespace, localName])?

@foolip
Copy link
Member

foolip commented Nov 24, 2015

That would certainly do the job, although it's unfortunate to waste memory on those arrays for the vast majority of attributes that aren't in a namespace. A mixture of strings and arrays isn't exactly perfect, either.

Any preferences, @esprehn?

@annevk
Copy link
Member

annevk commented Nov 24, 2015

The alternative is to continue with precedent: getAttributeNames() and getAttributeNamesNS(). Such joy.

@foolip
Copy link
Member

foolip commented Nov 24, 2015

Joy indeed, but that would probably work. One would lose the order between the two sets of attributes, but I'm not sure if that could possibly matter.

@foolip
Copy link
Member

foolip commented Nov 24, 2015

To clarify, with the attributes API one can see if an attribute with a namespace comes before an attribute without a namespace and vice versa.

@zcorpan
Copy link
Member

zcorpan commented Nov 24, 2015

The naming conflict issue is already there when using getAttribute(attributes[n].name). If people cared about it, they would use NS methods everywhere, but it seems like it's not really a problem.

@foolip
Copy link
Member

foolip commented Nov 24, 2015

@annevk clarified getAttributeNames() and getAttributeNamesNS() on IRC, both would return all attributes. I was assuming that the attributes would be split into two buckets.

@domenic
Copy link
Member

domenic commented Nov 24, 2015

I'd prefer just getAttributeNames() and wait for someone to ask for getAttributeNamesNS()...

@foolip
Copy link
Member

foolip commented Nov 24, 2015

I'd be OK with that, if the spec has a note saying that it's not guaranteed that the values will be unique, so you can't use it to implement something like Live DOM Viewer.

@annevk
Copy link
Member

annevk commented Nov 24, 2015

Yeah, this all sounds good. @smaug----, any concerns?

@smaug----
Copy link
Collaborator

So getAttributeNames() would return a new sequence each time?
Are we concerned about silly usage like
for (var i = 0; i < elem.getAttributeNames().length; ++i) {
// do something with elem.getAttributeNames()[i];
}

It is just that .attributes doesn't change, so fewer new objects there.

Would something like
readonly attribute FrozenArray attributeNames;
work, and in such way that it is replaced with a new array whenever an attribute is added or removed?

But I don't object getAttributeNames().

@annevk annevk closed this as completed in fb45d52 Nov 25, 2015
@annevk
Copy link
Member

annevk commented Nov 25, 2015

I went with getAttributeNames() and the hope that developers use it wisely. Adding another associated object to Element that needs to be updated correctly seems cumbersome.

@ArkadiuszMichalski
Copy link
Contributor

@annevk Return empty sequence when element has no any attributes is implicitly or should be explicitly defined in prose (like here: https://url.spec.whatwg.org/#dom-urlsearchparams-getall, https://fetch.spec.whatwg.org/#dom-headers-getall, https://xhr.spec.whatwg.org/#dom-formdata-getall)?

@zcorpan zcorpan reopened this Dec 3, 2015
@annevk annevk closed this as completed in 7b5e052 Dec 15, 2015
@annevk
Copy link
Member

annevk commented Dec 15, 2015

Thank you!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants