fix(:bug:): support use in Node.js with custom fetch (and no global)

[jgravois]

this tweak allows Node.js users to provide their own custom implementation of fetch to request() without polyfilling the global.

AFFECTS PACKAGES:
@esri/arcgis-rest-request

ISSUES CLOSED: #203

[coveralls]

Coverage remained the same at 100.0% when pulling 15e80a1 on custom-fetch-node into 98f7b34 on custom-fetch-headers.

[tomwayson]

I don't think the logic is correct here. I tried using node-fetch in the batch geocode demo and I get the following error:

Error: `arcgis-rest-request` requires global variables for `fetch`, `Promise` and `FormData` to be present in the global scope. You are missing `fetch`. We recommend installing the `isomorphic-fetch` modules at the root of your application to add these to the global scope. See https://bit.ly/2KNwWaJ for more info.
    at Object.request (/Users/tom6947/code/arcgis-rest-js/packages/arcgis-rest-request/dist/node/request.js:70:15)
    at bulkGeocode (/Users/tom6947/code/arcgis-rest-js/packages/arcgis-rest-geocoder/dist/node/bulk.js:42:34)
    at chunks.map.chunk (/Users/tom6947/code/arcgis-rest-js/demos/batch-geocoder-node/batch-geocode.js:94:7)
    at Array.map (<anonymous>)
    at parseCsv.then.data (/Users/tom6947/code/arcgis-rest-js/demos/batch-geocoder-node/batch-geocode.js:93:29)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

Also, I think that we need to change this line if (!options.fetch || !Promise || !FormData) { to if (missingGlobals.length > 0) {. I tried that locally on this branch, and it didn't change the above error (options.fetch is successfully passed in). I don't have time right now to fully debug, but I'll look later today.

[tomwayson]

ok, after digging a little deeper, I think the issue is that even when I supply a valid fetch implementation the the call to bulkGeocode(), which in turn passes that to request(), request() then calls authentication.getToken(url) and there is no way to pass options.fetch to that. If I console.log('options.fetch missing:', !options.fetch); right above this line, I see:

options.fetch missing: false
options.fetch missing: true

I suppose we could have getToken() accept an optional second argument that is either a fetch implementation, or a subset of IRequestOptions - importantly w/o authentication so that we don't create an infinite loop.

[tomwayson]

I'm thinking that the second parameter should be an options hash instead of just fetch() - unless we're sure that we're not ever going to need to pass any of the other options.

Given that getToken() ultimately calls the generateToken REST endpoint, and given that it's apparently possible to send that via GET, it looks like we'll ultimately need to support at least httpMethod, but I don't think we'll need his tagalong friend maxUrlLength and I don't know if we want to open the door to params.

We don't have to add any other options other than fetch at this time, I'm just suggesting it should be options instead of just fetch().

[jgravois]

I'm just suggesting it should be options instead of just fetch().

i agree.

[tomwayson]

Getting there, but, I don't think we should allow passing a full IRequestOptions - b/c if someone supplies an authentication it should go into an endless loop right? For now I'd make a new IGetTokenOptions w/ just fetch and httpMethod and then have IRequestOptions extend that.

[jgravois]

if someone supplies an authentication it should go into an endless loop right?

💡
👦

i've pushed up a few more commits to make the correction.

[tomwayson]

Man, I was this close to approving and merging, but one final pass over reveled the breaking change :(

[tomwayson]

This is a breaking change and I really don't want to get all wrapped up in that right now. Maybe we can avoid that by making this requestOptionsOrParams: (IGenerateTokenRequestOptions | IGenerateTokenParams) and then internally setting:

// TODO: remove this next breaking change and just have this take IGenerateTokenRequestOptions
const requestOptions = requestOptionsOrParams.params ? requestOptionsOrParams : { params: requestOptionsOrParams };

and then add this to #137

I know that's yucky, but feels better than dealing w/ a breaking change right now.

[jgravois]

i agree that a union type here is yucky and i also agree that its preferable to introducing a breaking change.

just pushed up a commit that uses a type guard because TS likes to make ternary operators even less readable.

[tomwayson]

🎉