greencatsoft / scalajs-angular

AngularJS Binding for Scala.js
Apache License 2.0
252 stars 42 forks source link

Angular element improvement #91

Closed svenwiegand closed 8 years ago

svenwiegand commented 8 years ago
  1. Make AngularElement injectable via$element`.
  2. Make creation of AngularElement from DOM elements and JQuery elements easier.
  3. Unfortunately JQuery (and thus AngularElement) do not provide functional operations for iterating about all DOM elements behind an AngularElement which is often required. This PR adds this feature.
mysticfall commented 8 years ago

Looks great to me. By the way, what about making IterableAngularElement really iterable, I mean by extending either from Traversable (i.e. from my other project) or even Seq as is done by org.scalajs.dom.ext.PimpedNodeList, for instance?

I think it would give us more flexibility if we can make it behave like a normal scala collection. Please let me know how you think about it. Thanks!

svenwiegand commented 8 years ago

Sounds like a good idea. I have an implementation where the implicit class extends Seq[dom.Element]. I would use dom.Element rather than AngularElement as type parameter due to performance reasons. One drawback would be, that I could not implement IterableAngularElement as a value class anymore, because Seq[T] isn't a universal trait.

What's your thought about this?

mysticfall commented 8 years ago

I had an impression that such overhead would be trivial, but it's possible that it's just my uneducated opinion.

How much of overhead would it cause, realistically? In terms of CPU usage, there's no DOM access or rendering involved in this case, so we only need to consider the time need for instantiating a new Scala.js object.

I don't have any exact numbers to back it up so I can be wrong, but I would be quite surprised if instantiating such a simple class or assigning a few variables in Javascript incur any perceptible overhead in a modern browser environment.

It might be a problem in extreme corner cases, like when it's invoked repeatedly in a loop, for instance. But I assume your intention in using Seq[dom.Element] rather than Seq[AngularElement] is to minimize the performance impact in such scenarios (because in that case, we only instantiate a Scala object for the parent node).

Maybe memory consumption is more of an issue, if we consider not using a value class here. But again, I doubt such a simple class instance would have any sizable memory footprint, especially if you are considering to use Seq[dom.Element] for its children without any wrapper.

So, I don't find any problem in not using a value class - and actually, I wouldn't object if you just use Seq[AngularElement] here. But as I said, I cannot claim I have in depth knowledge about Scala.js internals so I'm open to different opinions here.

svenwiegand commented 8 years ago

Not sure, how effetive JavaScript handles object creating and garbage collection, but I guess you're right. Will update the PR on the weekend.

svenwiegand commented 8 years ago

IterableAngularElement now is a Seq[Node].

mysticfall commented 8 years ago

Merged and published a new snapshot. Thanks for the contribution as always :)