stamen / modestmaps-js

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

1.0.0 alpha broken; can't initialize a new map #106

Closed migurski closed 12 years ago

migurski commented 12 years ago

I'm doing this:

        function tileURL(c)
        {
            return ['http://tile.stamen.com/terrain', c.zoom.toFixed(0), c.column.toFixed(0), c.row.toFixed(0) + '.jpg'].join('/');
        }

        var mm = com.modestmaps,
            map = new mm.Map('map', new mm.MapProvider(tileURL), new mm.Point(600, 400), []);

        map.draw();

And getting this: NOT_FOUND_ERR: DOM Exception 8: An attempt was made to reference a Node in a context where it does not exist.

What is the what?

migurski commented 12 years ago

Works in v0.22.0, by the way.

tmcw commented 12 years ago

See #102 - coerceLayer is removed, for the time being, so you need to provide Layers, all the time. It's in the 1.0.0 wiki, and isn't really broken - this is an API change.

migurski commented 12 years ago

Huh. Did not realize that the coerceLayer thing also broke initialization. Let's get 'er back! =)

tmcw commented 12 years ago

As a helper I'm open to it, as an API I think it's pretty bad. For instance,

var a = new providerthingy;
map.addLayer(a);
var b = map.getLayerAt(0);
console.log(a == b, 'wtf');

Like, this whole 'what you put in isn't what's in it' concept I think is a high cost for reverse-compatibility across major versions that don't necessarily need reverse-compatibility.

migurski commented 12 years ago

If you provide a Layer, then a will equal b, right? It's acceptable to me that the equality wouldn't shake out if you just threw in a simple provider, though I agree that giving it a layer should give you that layer right back.

How about 1.x uses your new behavior, but includes a sudoMakeMeAMap() or equivalent function that takes the same arguments as the pre-1.x mm.Map() constructor? Ideally, there'd be a search/replace-friendly way to get on board with 1.x that doesn't require a user to understand or use Layers.

Another thing that will make this a less bitter pill to swallow will be proper support for a hosted version of the library. I know you've argued against this before, but I think there's too much value in having a dumb-as-bricks way to get a map on a page without any caveats. Let's make code.modestmaps.com something we can push to and then use it for real.

tmcw commented 12 years ago

How about 1.x uses your new behavior, but includes a sudoMakeMeAMap() or equivalent function that takes the same arguments as the pre-1.x mm.Map() constructor?

Would the proposed MM.coerceLayer(x) function solve that need?

I know you've argued against this before, but I think there's too much value in having a dumb-as-bricks way to get a map on a page without any caveats. Let's make code.modestmaps.com something we can push to and then use it for real.

Yeah, cdnjs.com etc might be worth a look as well.

migurski commented 12 years ago

Would the proposed MM.coerceLayer(x) function solve that need?

It seems wordy, to me. Lots of exposed administrivia when a convenience function might address the same issue.

Yeah, cdnjs.com etc might be worth a look as well.

Ah, cool. Are they in it for the long haul? Might be easier to use something of our own with Cloudfront or Fastly or something

migurski commented 12 years ago

Where by “easier” I mean “harder, until the day cdnjs pivots their business model or gets bought.”

tmcw commented 12 years ago

It seems wordy, to me. Lots of exposed administrivia when a convenience function might address the same issue.

Meh? I'd rather not have an alternative map constructor just so that porting between 0.x to 1.x is a search & replace task. After all, it's code in one place or another it's either new MM.MapWithCoerceLayer(blah) or new MM.Map(coerceLayer(blah))

migurski commented 12 years ago

The convenience functions also offer a place for starting center and zoom to go, and fit with the way I tend to use MMaps Python with its mapByCenterZoom, mapByExtent, etc.

tmcw commented 12 years ago

Hate to be such a stickler about this, but what's the point of like

new MM.MapByCenterZoom(blah, new MM.Location(), 5);

instead of just

(new MM.Map(blah)).setCenter(new MM.Location()).setZoom(5);

The latter is just code already in Modest Maps and that's easy to know how to use post-initialization. I've got to side with bostock here and go against initializer growth, as good enough design of setters make it easy to supply as many initial parameters as you want, and also clear how to change them later.

migurski commented 12 years ago

So many parens, it looks like lisp. I see what it's doing but jesus. Also the first part wouldn't have a "new" on, it would be a convenience function that wrapped the constructor, and would sit alongside other convenience functions like "getMapByExtent" or "getMapByWoeid" (someday!).

I'm teaching a class right now where in order to get the students to understand Polymaps and D3, I am unrolling every single method chain so they can see what's happening and reason about it. Honestly I don't understand what's wrong with "initializer growth" when the alternative is a rats' nest of call chains - who is being hurt by additional functions that say what they do in their names, especially if the method-chained alternative remains on offer?

Want:

var map = getMapByCenterZoom('div-id', {lat: 37, lon: -122}, 9);
var map = getMapByExtent('div-id', {lat: 37, lon: -122}, {lat: 38, lon: -123});
// etc.
tmcw commented 12 years ago

Let's get back to the actual topic here: I'll work on a coerceLayer branch, which I think is enough to let legacy code get ported quickly. Initializer growth can happen outside of this repo if you want, or in an example. I don't think it should be in core.

migurski commented 12 years ago

It's not initializer growth. It's convenience functions. For convenience. Then you can have the initializer be as lean and method-chained and Bostockian as you want, and normal humans (like me, when I'm in a hurry) can continue to use Modest Maps without first having to mind-meld with John Resig.

The actual topic is that 1.0 breaks backwards compatibility with the initializer and I'm looking for ways to meet you halfway and reduce the pain without insisting that that we maintain continuity.

tmcw commented 12 years ago

Yes - hence MM.coerceLayer. It's a quick change and it'll probably be in by the end of the day. The disagreement about convenience functions or whatnot is a random tangent with no bearing on this; ticket it elsewhere.

migurski commented 12 years ago

Okay, cool.

I've pushed convenience functions in a new file, src/convenience.js.

tmcw commented 12 years ago

Dude... erg, does that need to be in core? Can't it be in examples/ or whatever? Also, these shortcut functions aren't true constructors, so can't be extended, and all of that jazz, and they're now the only examples of the dropped-brackets K&R style in the library at this point - something that we've been trying to standardize against with the usual Javascript style.

This also will need documentation, a changelog entry, etc. Really, I think this should live outside of MM and is just bloat.

tmcw commented 12 years ago

And see #96 for discussion of coerceLayer initially.

migurski commented 12 years ago

I'd really prefer if it was, yeah.

On the benefit side, my thinking is that the functions are there for someone who just wants a map with a minimum of fuss, and I consider it fuss to have to look for examples, load extra packages, etc. My usage of MMaps tends to be of the "just make me a stupid map already" variety (like the one at the bottom of pages like this http://walkingpapers.org/print.php?id=g58nclp7), so I'm not a fan of anything that increases that friction through requiring extra loads. I know it's easy to load extra stuff, but it's not always easy to know that you have to, and I'd love for it to be possible for new/naive/impatient users to have a map with a single script src and a single function call:

<script src="modestmaps.min.js" type="text/javascript"></script>
<div id="map"></div>
<script type="text/javascript">
MM.mapByCenterZoom('map', 'http://tile.stamen.com/terrain/{Z}/{X}/{Y}.jpg', {lat: 37.804, lon: -122.271}, 15);
</script>

On the bloat side, I believe that these convenience functions add little in the way of actual weight (355 bytes in min.js), introduce no new demands on the API, and allow you to freely develop the Map constructor as you see fit to work with the layer features. It's an on-ramp, essentially, one that we can maintain for the benefit of the impatient.

The /examples directory is a total graveyard. I'm happy we have it but I only go there to leave flowers and I wouldn't recommend it as a necessary starting point for anyone.

Also: we're trying to standardize against dropped brackets? I did not receive the memo. I love me some dropped-ass brackets.

tmcw commented 12 years ago

If MM.Map had the small amount of code necessary to make new optional, that would look like

MM.Map('map', new MM.TemplatedLayer('http://tile.stamen.com/terrain/{Z}/{X}/{Y}.jpg')).location({lat: 37.804, lon: -122.271}).zoom(15);

For newbie users, there are dumber additions we should do: number one is documentation. In-core syntax sugar comes last, not first.

Yes; dropped brackets have been gone for a few months. See 8604096f36aaf453648f604c6146342725884233 and others. The JS bracket consensus is solidly on non-dropped K&R style.

migurski commented 12 years ago
MM.Map('map', new MM.TemplatedLayer('http://tile.stamen.com/terrain/{Z}/{X}/{Y}.jpg')).location({lat: 37.804, lon: -122.271}).zoom(15);
MM.mapByCenterZoom('map', 'http://tile.stamen.com/terrain/{Z}/{X}/{Y}.jpg', {lat: 37.804, lon: -122.271}, 15);

Blargh, it requires a user to understand two classes and two methods (or one class, one function and two methods without "new"), instead of one function. It’s not horrifically longer, but there’s so much more to grok in there—method chains, noun-named methods instead of verbs, etc.

I’m honestly trying not to be difficult here. I feel like we’re sacrificing approachability at the altar of JS trends that I’m just not on board with. I understand the appeal of method chaining and verb avoidance, but as someone who doesn’t spend 16 hours a day working in the language I’m also sensitive to the need for APIs that provide clear, unambiguous entry points with self-describing names. “mapByCenterZoom()” is an example of that, I believe.

I’d love to learn more about the bracket consensus by the way. I switched from non-dropped to dropped sometime around 2005 after finding that I was sticking an extra newline where the bracket gets dropped for readability.

straup commented 12 years ago

For what it's worth, as a "developer" who is meant to use this stuff (and as someone who dislikes and systematically dismantles method chaining) I'm with Mike. For all the reasons he's outlined above.

(Except for the thing with the curly braces.)

tmcw commented 12 years ago

Method chaining either way; I'm not even strictly against using something like mapByCenterZoom(), I just haven't seen the purpose of core modestmaps being shortcuts - so far, we've been making the building blocks for other stuff to go on.

Having a file with mapByN convenience methods isn't necessary for MM, and is easily implemented elsewhere, at the cost of a script tag include: not exactly a massive inconvenience. Otherwise, there'll be plenty of users (myself, most of the maps we make) who have essentially dead code in their library.

migurski commented 12 years ago

Surely you can live with 0.35KB of dead code? The purpose is largely about approachability (for the new) and easier compatibility (for me, basically). I promise I’ll shut up if I can use Modest Maps without having to stay up to date on your wiki edits and commit history. =)

tmcw commented 12 years ago

All right, well off to #102.