Make NodeList seqable and add non-macro versions of key methods.

[pangloss]

Making NodeList seqable cleans up the implementation and makes it much easier to work the DOM with or without Dommy's special methods.

I added non-macro versions of by-id, by-class and by-tag, plus added a tag-name method because I needed all of those methods on my next project!

[pangloss]

This pull currently breaks a couple of tests which I'll fix before it's ready to merge. Please let me know if you have any objections or other thoughts regarding it!

[aria42]

Thanks for the PR. Why would you want the non-macro version, what use-case did you have that made these worthwhile? As far as I can tell, there is no reason not to use the macro

[pangloss]

I needed the non-macro version because by-class was using the name of the symbol I was passing into it instead of the symbol's value! I updated the others at the same time without giving them too much thought.

Here are 2 use cases that won't work with macros:

(doseq [class [:good :bad :ugly]](toggle! %28by-class class%29))

(->> tags (map by-tag) (mapv toggle!))

In the case of these simple methods, I don't think macros are buying you anything at all, but there are some small disadvantages.

Cheers! Darrick

[cpetzold]

I like what you're doing with clobber, and I think these changes are fine.

If we go with using your protocol extensions, we no longer have a use for ->Array. Can you clean that up and try rebasing/testing again? We recently changed to using clojurescript.test instead, so testing should be simpler.

Thanks!

[pangloss]

Ok, great! Might be a week or two before I find time to update this pull request, though.

[swannodette]

Just your friendly reminder that extending HTML natives in libraries is not a good idea regardless of the convenience :) Applications developers can do this themselves. If a library does this then it's going to come as a big surprise to applications developers who have their own ISeqable implementation for NodeList.

[pangloss]

I think that by discouraging the use of a library like Clobber, we just force each developer to create their own versions of these same extensions or otherwise work around the existence of the hodgepodge of specialized collection classes returned by DOM operations. From a quick GitHub search, I see there are already (but only) a couple of libraries that have extended NodeList with either ISeqable or ISeq. I think the best approach is to simply encourage those projects and others to standardize before any significant body of legacy code accumulates.

My goal in moving those extensions into a Clobber is to make it easy for these extensions to be shared and for the community to develop a standard implementation that can evolve to be robust across environments. It's a great step forward that Dommy will hopefully be the first of many to adopt it.

[swannodette]

@pangloss I'm not understating when I say would never use a library that does this and certainly would not recommend anyone use a library that does. I'm already on record for saying as much on the various Clojure(Script) channels. Since protocols day one - Rich Hickey has stated do not extend types to protocols if you don't control the type or the protocol. In applications it's understandable - in libraries it's just irresponsible.

As an alternative that does not suffer from the problems I've already outlined - libraries can provide helper functions that extend specific instances of NodeList via reify or specify.

[jeluard]

Probably this should be a user choice and not something imposed by a dependency.

Also it's not clear to me what happens if one dependency extends to ISeqable and another to ISeq. Similarly what if my app includes transitively several versions of clobber, possibly with different version of this particular extension?

I'd rather dommy not introduce a dependency on clobber or any other library actually.

[pangloss]

Hmm, perhaps the route of extending instances with specify would achieve the same improvement that I was aiming for originally without causing problems with other libraries. It would be easy to change clobber to do that instead.

@swannodette, @jeluard would that ease your concerns?

[swannodette]

@pangloss we would need to add specify! to ClojureScript but I'm not against that. specify only works on instances of ICloneable. Feel free to open a ticket in JIRA and I will get it in by the next release.

[cpetzold]

Good points, I hadn't thought of the implications of extending natives!

@pangloss I agree that we shouldn't do this until it can be isolated with something like specify, however the rest of your PR (the non-macro selectors and boulding-client-rect fix) looks fine. If you can break these out into their own change set, they can be merged separately.

[jeluard]

Using specify sounds definitively great.

Now I am not sure how ClojureScript behaves when several version of the same library are used in a single project. Ideally a basic library like dommy would be self sufficient to reduce potential collision.

That's more a general feeling, nothing specific to clobber.

[pangloss]

@cpetzold I'll do that.

@swannodette JIRA created. The changes to clobber turn out to be trivial and will definitely be an improvement. Thanks.

[cpetzold]

Sounds good!

[swannodette]

@pangloss specify! landed in ClojureScript 0.0-2156

[cpetzold]

Wow, nice turnaround!