Becklyn / mojave

A library of commonly used JavaScript tools and helpers by Becklyn
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Add hasAttr and hasData #106

Closed keichinger closed 6 years ago

keichinger commented 6 years ago

Sometimes it's useful to check whether a given attribute is just set, especially for marker attributes used by some frameworks and sites.

This PR adds support for this with hasAttr(element, key) and hasData(element, key).

Side effect: with hasAttr() it's also possible to check for data-* attributes.

apfelbox commented 6 years ago

Do we really need this in Core? I mean, both of them are essentially null != get*() ...

(And that’s how they should be implemented btw, instead of mostly duplicating the get*() functions)

apfelbox commented 6 years ago

Actually, I will probably remove getAttr in the next major version, as the function in mojave is rather useless.

keichinger commented 6 years ago

For a simple „is that attribute set” I'd find it easier to read and reason about the code's functionality when it's a dedicated method.

About the re-implementation: That's true, but there's IMO an important thing to notice that element.dataset[index] returns undefined instead of null, which makes the comparisons for getData and getAttr more ugly than a dedicated method.

apfelbox commented 6 years ago

And lastly, both implementations are wrong. You should use hasAttribute and probably containment checks for the object in hasData().

Consequently, you should use element.hasAttribute() to check for an attribute's existence prior to calling getAttribute() if it is possible that the requested attribute does not exist on the specified element.

https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute

I would remove hasAttr - because it’s doesn’t add anything - and change hasData to check for existence and not null (in contrast to my first comment).

apfelbox commented 6 years ago

For a simple „is that attribute set” I'd find it easier to read and reason about the code's functionality when it's a dedicated method.

hasAttr(element, „test“)

is imo way less readable than

element.hasAttribute(„test“)

(Writing on an iPhone on Github is hell)

keichinger commented 6 years ago

Well, the existence of element.hasAttribute changes a lot. Don't know why I didn't think of checking for that method first.

Obviously I just wanted to write some tests... :P

I'll refactor as you said.

apfelbox commented 6 years ago

Do you mind soft deprecating getAttr as well?

keichinger commented 6 years ago

Does that include only the @deprecated JSDoc annotation or also an console.warn at the beginning or the method?

apfelbox commented 6 years ago

Soft deprecation is only the doc. Hard deprecation would be throwing warnings around. 😉