v1.0.0 refactor

[fabriziomoscon]

No description provided.

[fabriziomoscon]

@moshen this is the BIG refactory I prepared over the past months. It would be good to see what you think. I would need more stuff, but I figured that this is workable already.

My plan is to use this on my production servers then promote it to npm v1.0.0

[moshen]

I noticed #71 and #72, but lib/elevationFromLocations.js also needs the same params object treatment.

[fabriziomoscon]

I have refactored this lib/elevationFromLocations.js so it accepts a params and callback like the other functions in the library.
I have also noticed that the geocode and reverseGeocode functions where not checking for required parameters, so I have added the checks and the relative tests

We could spent more time clean it completely and refactor checkAndConvertArrayOfPoints, checkAndConvertPoint, move elevationFromPath into an external file and clean encodePolylines.js, but I think those can wait to land into a patch.

Also once this is done there are changes that Google made to the distanceMatrix API that I wish to include.

[moshen]

Just noticed the staticMap call didn't get the treatment either.

You're right about refactoring the utils. Though, I think any API breaking changes should go in before release.

[fabriziomoscon]

I could work on the first three

• refactor static map signature
• refactor street map signature
• refactor elevationFromPath signature
• change config key names

[moshen]

FYI, pushed the config-key changes

[fabriziomoscon]

@moshen I refactored the staticMap, I have added plenty of unit tests because of all the many parameters, but I need to make the callback not mandatory for users that only need the URL

Also I am not able to make it work with a path and encode_polyline:true (which might have been the case for v0). Maybe that is something you could have a look at, because I didn't implement the initial version of that function.

[moshen]

I need to make the callback not mandatory for users that only need the URL

Seems easy enough: remove this check, as you are already checking if callback exists in the makeRequest function.

In the original version any call could be made without a callback to simply return the uri (and not make the request). It certainly looks like that has changed. I guess the question is to make a callback necessary for all calls, some calls or no calls. Making it necessary for all calls is probably good for developer productivity (catching errors early), but in the case of staticMaps, sometimes you may just want to return the map uri (maybe in an img tag?). Thoughts? You seem to have already weighed in by making callback required for every call.

Also I am not able to make it work with a path and encode_polyline:true (which might have been the case for v0). Maybe that is something you could have a look at, because I didn't implement the initial version of that function.

I'm not sure what the issue is. When I run the tests on both branches they seem to work. Do you mean just encoding for staticMap calls or any encoded polyline?

[fabriziomoscon]

Yes, it is a fair use case to get the URI of the static map without the binary. The above commit makes that happen.
I have also fixed an issue where I didn't use multiple markers parameters in case the style changed.
Updated the README.md as well with the new API example for staticMap.

The encode_polyline:true seems to work now, not sure what was the issue.

[fabriziomoscon]

@moshen I pushed the last change as agreed, please have a look and let me know

[lastquestion]

@fabriziomoscon , looks like you're doing a big refactor. Did you change stagger-time so it works on all request kinds in your branch? I'd like to request / add that in. Basically, all requests go into a job queue, and a worker sends the request, and then sleeps stagger-time before picking the next. I implemented a very surgical change in the current master branch, which I guess I'd like to also hear from @moshen if it's worth / interesting to share that back? (I would open a new issue / PR for that separate from this, obv)

[moshen]

@lastquestion That's a good question.

Did you change stagger-time so it works on all request kinds in your branch?

No.

I'd like to request / add that in.

It can be an addition to the api (optional), that could go into 1.1.0, trying to concentrate on api breaking changes for this release.

Basically, all requests go into a job queue, and a worker sends the request, and then sleeps stagger-time before picking the next.

As far as I understand it, currently this isn't how it works. Only the request initiation times are staggered, the current implementation doesn't wait for requests to complete before initiating more.

Also, waitress will probably be replaced by more standard usage of something like Promises (bluebird) or Observables (RxJS). Which would make these types of changes pretty easy.

I don't know if the queuing of requests within the api is the way to go though, as the upcoming changes will make it easier to wrap the api yourself to throttle requests.

@fabriziomoscon , thoughts?

[lastquestion]

@moshen : from what I understand it only throttles on one specific kind of request elevationPath, e.g. https://github.com/moshen/node-googlemaps/blob/master/lib/googlemaps.js#L267 but not anywhere else.

My implementation also only staggered the requests, and didn't wait for replies, but it did it also for queries like geocode which per google has a 5 per second rate limit for public use.

I think you're right that if the request object or whatever is exposed, users can manage their wait/throttle/whatever themselves. Factoring it out from the library seems the right approach and simplifies code for everybody including clients.

[fabriziomoscon]

I agree with factoring out of this library any additional generic functionality such as backing off and queuing requests. Keeping the library to bare minimum will increase maintainability and reduce complexity. I think respecting the modularity principle which is at the at the core of npm and node.js and io.js will bring lots of benefits to the library, and will allow anybody to extend it.

@lastquestion, in regards to throttling keep in mind that users are able to negotiate custom quotas per APIs directly with Google in order to satisfy their business needs, therefore having a super generic config such as stagger-time for those users would be too simplistic and not fit for purpose.

[moshen]

from what I understand it only throttles on one specific kind of request elevationPath, e.g. https://github.com/moshen/node-googlemaps/blob/master/lib/googlemaps.js#L267 but not anywhere else.

100% Correct

I think you're right that if the request object or whatever is exposed, users can manage their wait/throttle/whatever themselves. Factoring it out from the library seems the right approach and simplifies code for everybody including clients.

That's not really necessary. The bigger bonus is that the api calls will all have the format: gm.apiCall(paramsObject, callback).

@fabriziomoscon

I agree with factoring out of this library any additional generic functionality such as backing off and queuing requests. Keeping the library to bare minimum will increase maintainability and reduce complexity. I think respecting the modularity principle which is at the at the core of npm and node.js and io.js will bring lots of benefits to the library, and will allow anybody to extend it.

I think that this is the right way to go. Maybe including an example (at a later date) of throttling api calls without touching the api.

[fabriziomoscon]

@moshen I have tested few methods in my code base and so far I haven't found issues, unless you want to test more I would consider merging this code and release on npm

[serhalp]

Are there plans to merge this in anytime soon?

[fabriziomoscon]

Hi @serhalp it is in my list of TODOs to merge this, but if you have a production environment to deploy this branch to please go ahead so we have more than one production test.