API method signature refactor

[jgravois]

new API design

1. IParams is moved inside requestOptions

request(url as string, params as IParams, requestOptions?)

// becomes

request(url as string, requestOptions?)

2. we recommend that folks extend requestOptions to include additional arguments

export interface ICustomRequestOptions {
  httpMethod?: string
  authentication?: IAuthenticationManager || UserSession || ApplicationSession
  routeModifier?: string,
  anotherFirstClassArg?: any,
  params?: {} as IParams // backdoor
  etc?: anything we desire
} extends IRequestOptions

criticism welcome, but it felt clunky to me to have all higher level wrapper methods require the same options object to be passed because some of them only really need one simple input.

because of this, i came up with yet another rule of thumb proposal for this library.

a. if a method can be executed successfully anonymously
b. and it only usually requires one piece of information from a developer

one custom parameter will be mandatory and followed by an optional requestOptions object.

method(boolean || string || etc, requestOptions?)

if, for any reason, requestOptions is mandatory, everything will be stuffed into a single object.

method(requestOptions)

i like this for several reasons.

1. the API design is still generally consistent.

   request(url, requestOptions?)
   
   // looks similar to 
   
   geocode([-118,34], requestOptions?)

2. it makes for more concise methods with less property names to remember.

   geocode([-118,34])
   getItemById("j323h")
   
   // instead of
   
   geocode({
     coordinates: [-118,34]
   })
   
   getItemById({
     id: "j323h"
   })

3. we no longer need to re-order arguments depending on whether or not requestOptions is required.

   // anonymous
   genericMethod(arg, arg2?, requestOptions?)
   
   // requires authentication
   genericMethod(arg, requestOptions, arg2?)

4. we avoid dead slots entirely

   // no more of this
   genericMethod(arg, null, {}, foo)

the not so good

there is one thing (already) that i'm not incredibly fond of.

putting params inside requestOptions is definitely a convenient catch-all, but it makes for an ugly signature when the wrapper author (in this case me) has only stubbed out some of the options the endpoint supports.

// IAddress passes through some, but not all of the parameters the API exposes
geocode({
  address: "380 New York St",
  postal: 92373
},
{
  params: {
    outSR: 3857
  }
})

caveats

i have not updated the TypeDoc yet and the code is still rough in several places, but i decided it would be prudent to save polishing until after we've had another round of discussion.

whatever becomes of this, we should land #77 first. i'm happy to rebase on top of it if we end up green lighting this work.

[jgravois]

i'm really annoyed to see pat's excellent commits from #77 polluting the diff here.

my expectation was that merging that PR and then merging master into this branch would allow me to resolve the merge conflict and not show commits that are already present in the base branch.

[patrickarlt]

@jgravois if you close and re-open this PR it should display fine. GitHub displays stuff like this kinda weird master...r/options-object.

[tomwayson]

This LGTM - I like your rule of thumb. I don't see the "Not so good" being a big deal. In your example, isn't just the case that outSR should be added to IAddress, or am I missing something?

[tomwayson]

I should say the concept LGTM - I haven't reviewed the code yet, which I'm sure is top notch.

[patrickarlt]

@jgravois this looks good and I think the new standards for the API make sense. I would document this in the wiki somewhere though we need an "API Design" topic for similar.

I also think we should decide what we call the second (or first) option. I see param, options, requestOptions and whateverOptions used interchangeable so we should probably sync this up at some point.

[patrickarlt]

There are no methods in the API that only accept an application login. We can probably safely remove this.

[patrickarlt]

@jgravois I think it might be a little confusing to call this params and also have params in all the options objects. Maybe the signature should be (url: string, options: IFetchTokenParams)

[noahmulfinger]

a. if a method can be executed successfully anonymously
b. and it only usually requires one piece of information from a developer

one custom parameter will be mandatory and followed by an optional requestOptions object.

@jgravois What if we were to use method overloads to deal with that case? Something like:

function geocode(address: string, requestOptions?: IGeocodeRequestOptions): Promise<IGeocodeResponse>;
function geocode(address: IGeocodeRequestOptions): Promise<IGeocodeResponse> {
    if (typeof address === "string" {
        // do singleline geocode
    } else if (typeof address === "object") {
        // pull address params from requestOptions.params
    }
}

This would also remove the not so good case because you can just type the requestOptions.params to be IAddress and just add [key: string]: any to the IAddress interface. Then users would geocode with:

geocode({
    params: {
        address: "380 New York St",
        postal: 92373,
        outSR: 3857
    }
});

Some caveats:

• you can't necessarily require any address fields in the second overload case
• naming of the input parameters has to match even if they are very different types, which is a little odd

Overall, I like the pattern you've set up, I just thought I would put this out as a possibility.

[jgravois]

all excellent suggestions. i'll get to work on implementing them, tidy things up and hope @dbouwman doesn't knock let the air out of my balloon when he gets back from 🏄

[tomwayson]

@noahmulfinger I was thinking about a similar idea, though using union types instead of overloads b/c I thought that overloads are only needed if the type of the function's return is different as per https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#use-union-types

function geocode(address: string|IGeocodeRequestOptions): Promise<IGeocodeResponse> {
    if (typeof address === "string" {
        // do singleline geocode
    } else if (typeof address === "object") {
        // pull address params from requestOptions.params
    }
}

I'm not totally sure that idea checks out tho.

[patrickarlt]

@jgravois @noahmulfinger @tomwayson we might be over complicating Johns example here so how about this new rule:

If a method accepts 2 params the first param cannot be an object. Move all keys in that object into the options object.

This means that stuff like this is out:

geocode({
  address: "380 New York St",
  postal: 92373
},
{
  params: {
    outSR: 3857
  }
})

and you can geocode like what @tomwayson suggested and do this:

interface IGeocodeRequestOptions extends IRequestOptions {
  address?: string;
  address2?: string;
  address3?: string;
  neighborhood?: string;
  city?: string;
  subregion?: string;
  /**
   * The World Geocoding Service expects US states to be passed in as a 'region'.
   */
  region?: string;
  postal?: number;
  postalExt?: number;
  countryCode?: string;
  outSr: number;
}

function geocode(address: string|IGeocodeRequestOptions): Promise<IGeocodeResponse> {
    if (typeof address === "string" {
        // do singleline geocode
    } else if (typeof address === "object") {
        // pull address params from address
    }
}

which then means I can do this:

geocode('221B Baker St, London');

geocode('221B Baker St, London', {
  outSr: 102100
});

geocode({
   address: "221B Baker St",
  city: "London",
  outSr: 102100
});

[noahmulfinger]

@patrickarlt That rule makes sense to me. The only complication is that, to allow the function calls you listed, the interface and function have to look like the following:

export interface IGeocodeRequestOptions extends IRequestOptions {
   params?: {
      address?: string;
      address2?: string;
      address3?: string;
      neighborhood?: string;
      city?: string;
      subregion?: string;
      /**
       * The World Geocoding Service expects US states to be passed in as a 'region'.
       */
      region?: string;
      postal?: number;
      postalExt?: number;
      countryCode?: string;
      outSr: number;
   }
   authentication?
   etc?:
}


function geocode(address: string|IGeocodeRequestOptions, options?: IGeocodeRequestOptions)

Which would allow the user to pass in two options objects.

[patrickarlt]

@noahmulfinger not at all. Any param can be encoded inside IWhateverRequestOptions which I proposed in #75. Use of params is only for additional API parameters that you want to pass to the API that we do not specifically handle.

So the full interface for IGeocodeRequestOptions looks like

interface IGeocodeRequestOptions extends IRequestOptions {
  address?: string;
  address2?: string;
  address3?: string;
  neighborhood?: string;
  city?: string;
  subregion?: string;
  region?: string;
  postal?: number;
  postalExt?: number;
  countryCode?: string;
  outSr: number;
  authentication: ...; // from IRequestOptions
  httpMethod: ...;  // from IRequestOptions
  params: ...;   // from IRequestOptions any additional params we didn't specify in `IGeocodeRequestOptions` like category, extent ect....
  fetch: ...; // from IRequestOptions
}

function geocode(address: string|IGeocodeRequestOptions, options?: IGeocodeRequestOptions): Promise<IGeocodeResponse> {
    if (typeof address === "string" {
        // do singleline geocode, merge applicable extra params from the second optional `options` param and `options.params`
    } else if (typeof address === "object") {
        // pull address params from options, merge with additional params from options.params
    }
}

[noahmulfinger]

@patrickarlt Ah okay, I misunderstood how that object worked.

Also, to allow this function call:

geocode('221B Baker St, London', {
  outSr: 102100
});

We would need the function definition to look like this:

function geocode(address: string|IGeocodeRequestOptions, options?: IGeocodeRequestOptions)

That seems reasonable. We would just ignore the second param if the first param is an object.

[patrickarlt]

@noahmulfinger Yup updating my example to make that clearer.

[tomwayson]

I don't know the second IGeocodeRequestOptions in:

function geocode(address: string|IGeocodeRequestOptions, options?: IGeocodeRequestOptions): Promise<IGeocodeResponse>

Feels a little weird to me and not worth it just to support:

geocode('221B Baker St, London', {
  outSr: 102100
});

I mean, that's a nice signature, and I'll admit it's better than:

geocode({
   address: "221B Baker St",
   city: "London",
   outSr: 102100
});

But personally, I don't think it's worth the head scratching that people will do when they see something like this in the docs:

function geocode(address: string|IGeocodeRequestOptions, options?: IGeocodeRequestOptions): Promise<IGeocodeResponse>

We'll have to explain that the second request options will be ignored if you pass 2 of them - for many functions.

Part of what's confusing to me is that in this particular example, the string argument is a full address, but no such param exists on the IGeocodeRequestOptions.

So, if users could do this:

geocode({
   fullAddress: "221B Baker St, London",
   outSr: 102100
});

Then I'd say we should abandon the second argument entirely and just go w/:

function geocode(address: string|IGeocodeRequestOptions): Promise<IGeocodeResponse>

All that said, if everyone likes the second options arg, I'm game. I don't have strong feelings about this, just a feeling.

I'm going to noodle around w/ some feature service functions and see how they feel.

[jgravois]

update: test coverage is 100% (again), TypeDoc is in sync and i'm more or less happy with the overall refactor.

i'm with @tomwayson regarding the optional second IGeocodeRequestOptions argument slot. i don't think the complexity it adds to the internal method or the documentation justifies the additional cool signature it allows for.

to do:

• add loop to check the existence of each first class named parameter prior to passing them through to geocode() (this is sugar, lets sprinkle later)
• refactor searchItems() to expect string | ISearchRequestOptions

[patrickarlt]

@tomwayson @jgravois I'm fine with:

function geocode(address: string|IGeocodeRequestOptions): Promise<IGeocodeResponse>`

Lets get this merged.

[jgravois]

Lets get this merged.

@patrickarlt agreed. once @tomwayson looks over this w/ @dbouwman eyes, i'd be ready to publish v1.0 on npm