search

stamen / modestmaps-js

Modest Maps javascript port
http://modestmaps.com
566 stars 152 forks source link

Switch to object-builder functions rather than new for some functions #47

Closed tmcw closed 12 years ago

tmcw commented 13 years ago

Main layout might look like:

still using new

RandomEtc commented 13 years ago

Performance-wise, I think it makes more sense to use new for Coordinate/Point/Location than it does for Map. Here's the discussion from last time: https://github.com/stamen/modestmaps-js/issues/22

I think there's a strong advantage to keeping prototypes for everything, mainly consistency of implementation. I'd be persuaded otherwise by big performance gains (including on IE8... otherwise why not use polymaps) or big size reduction.

In general though, isn't this just yak-shaving? What's the goal?

tmcw commented 13 years ago

As I said before, this isn't really my focus at all - I've been working on the transforms-refactor branch recently, not less-new - what sparked this was this comment where I started thinking that we could eliminate some of the get*Handler stuff as well as mm.bind and replace it with something more straightforward. I think it makes a lot of sense for handlers to adopt this scheme because otherwise they need hacks to mess around with this. The rest I'm less opinionated on.

But, my main point is: this is not my main push right now - there's other refactoring to do. I'm opening this mainly to de-derail #8.

RandomEtc commented 13 years ago

Understood. Hopefully pulled #8 back on track :)

I just wanted to point out that this is basically the same issue as #22 and that we decided to drop it back then (just a couple of months ago). Let's re-focus this issue on Handler implementation if that's something you would like to clean up next.

tmcw commented 12 years ago

Yeah - no actions here, I'll start a new issue for handler improvements.