Allow customizing request() options

mapbox / geojson-mapnikify

Transform GeoJSON objects into Mapnik XML stylesheets with embedded GeoJSON data and simplestyle-spec-derived styles.

ISC License

38 stars 22 forks source link

Allow customizing request() options #31

Closed agius closed 6 years ago

agius commented 6 years ago

Currently, there is no way to customize the options used for fetching url-based image markers. This can be a problem if a parent library or application wants to set a longer or shorter timeout, specify a proxy server, or other special use cases. This PR allows setting a custom request() client, where these options can be specified using request.defaults() .

Also noticed that tests were hitting Flickr urls which could disappear at some point - fixed that by mocking those requests with nock

coveralls commented 6 years ago

Coverage increased (+3.1%) to 80.667% when pulling f4f70f1b5af8c028717fb83005bcf149725a014b on request-defaults into 3f02bb0824c40434f36de9b4e70b24660885a558 on master.

GretaCB commented 6 years ago

@agius Thanks for spinning this PR up! The tests look 👍 , and I just added a couple clarifying comments.

agius commented 6 years ago

@GretaCB thanks for the review! I've incorporated y'all's feedback. QQ: should I bump the package version in this PR, or merge and make a separate bump PR? Nothing backwards-breaking, so this'd prob be a patch or minor release?

agius commented 6 years ago

@GretaCB / @flippmoke - ping for re-review? also, per above, should I bump package version here, or separate PR? Thanks!

GretaCB commented 6 years ago

Based on semver docs, I'd say minor bump since this PR is adding functionality. I'm also trying to figure out exactly which repos use this as a dep, just so we can do a bit more upstream testing. ...I'll be back

GretaCB commented 6 years ago

I installed this branch into tilelive-overlay, and ran the tests successfully 👍 .

agius commented 6 years ago

@GretaCB thanks for the follow-up! Bumped the version - good to merge & release?