sjlu / cities

Lookup cities based on zipcodes or GPS coordinates
https://www.npmjs.com/package/cities
27 stars 15 forks source link

Actually I forked it and fixed it #2

Closed Routhinator closed 10 years ago

Routhinator commented 11 years ago

I'll do a pull request in a few..

The dependencies section in the package.json file is for modules the this package depends on in order to function. Your module has no dependencies, and thus this should be blank.

The nodeJS versioning problem was because you explicitly declared 0.8 instead of requiring a minimum of 0.8 .. 0.10 is newer than 8.

Prepending

Routhinator commented 11 years ago

Prepending >= before a package defines minimum rather than an explicit dependency

sjlu commented 11 years ago

Take a look at server.js, it contains the following lines.

var express = require('express');
var app = express();

It is used for running a web node when needed and especially when I launch it with Nodejitsu.

Routhinator commented 11 years ago

Damn, sorry I didn't realize this created a new issue. I meant to reply to the OP.. Github issues don't work well from mobile.

I just saw what you mean.. but now I'm confused.

The server.js creates some endpoints. However those shouldn't really be part of the core module, perhaps a 'resource' directory with examples? I use restify. I disabled the express include and it works for me. I wouldn't want to have express in my server stack, including two REST frameworks would be bad.

Ideally I think the module should be generalized and inclusion resources can be added for people that want them. Some people won't want a /zip endpoint automatically added to their API when installing a module for example.

Routhinator commented 11 years ago

Also, anyone can correct me if I'm wrong, but a server.js should never live inside a module, only in the server you are building..

sjlu commented 11 years ago

So I do understand where you're coming from. However, I believe this is more of a preferential architectural conversation rather than what's right and what's wrong.

From my standpoint, I believe server.js should stay where it is as your code that imports this module would never touch its version of express or that file itself. Yes, its an extra "x" amount of kilobytes on disk but it is very negligible.

I would consider breaking this out and making a cities-server repository to break this out, but I don't think its necessary.

Routhinator commented 11 years ago

Hmm, well I don't think it needs to be broken out, just put into an 'extras' subfolder as it is technically an 'extra' - if someone wanted to use it they would have to take the code from within it and add it to their server.js structure or do an include to load this server.js - Putting in extras also identifies it as an unnecessary part that can be removed from the node_modules/cites/ directory if someone is looking to reduce the size of their project to a minimum.

In the end though it's up to you. I had to make more changes to the fork I made, as the DB in this project only has US cities, which made it no good to me as it was. I've worked in a gzipped JSON of the MaxMind global cities and restructured it quite a bit in my fork. We can see about working the two projects back together if you like, or if you prefer I can just keep developing my fork and release it as a seperate project. The world city DB does make the project much heavier.

sjlu commented 11 years ago

Would like to see what you have, I think being able to support multiple city DBs and toggling which one to use would be awesome.

Send a pull request out to me when you think you got something ready :).

sjlu commented 11 years ago

On a side note, I also use this in my sjlu/meteorologist project which works great as a module with no problems with server.js.