Esri / arcgis-rest-js

compact, modular JavaScript wrappers for the ArcGIS REST API
https://developers.arcgis.com/arcgis-rest-js/
Apache License 2.0
347 stars 119 forks source link

Param builders for building complex parameter objects #384

Open patrickarlt opened 5 years ago

patrickarlt commented 5 years ago

Given that this library now has the capacity to do routing https://github.com/Esri/arcgis-rest-js/pull/382 and create feature services I want to put forward the idea of including some reasonable APIs for building the parameters objects like what I specced for https://github.com/Esri/esri-leaflet-routing/blob/master/Route.md and https://github.com/Esri/esri-leaflet-routing/blob/master/TravelArea.md. THis would make the job of building out the params or options objects much more reasonable for things like IAddToServiceDefinitionRequestOptions to wrap common use cases. For example:

import { addToServiceDefinition, layerBuilder } from '@esri/arcgis-rest-feature-service-admin';

const layer = layerBuilder()
  .field({/*...*/})
  .field({/*...*/})
  .field({/*...*/})
  .spatialReference(4326)
  .enableEditing();

addToServiceDefinition(serviceurl, {
  authentication: userSession,
  layers: [layer]
});

Opening this up for comment, @noahmulfinger @araedavis @tomwayson @dbouwman @jgravois.

dbouwman commented 5 years ago

Having fluent builders would certainly help us bury more platform idiosyncrasies, so that alone is a win.

import { search, queryBuilder } from '@esri/arcgis-rest-items';

// ability to construct a template that can later be refined... 
// a sort of partial-application on an object graph
const texasWaterWebApps = queryBuilder()
   .withAuth(myAuthFromSessionOrWherever)
   .hasType('Web Mapping Application')
   .withoutType('Web Map')
   .hasTag('water')
   .hasTag('texas')
   .culture('en-us')
   .hasKeyword('hubSolution')
   .withoutKeyword('hubSolutionTemplate');

// later... given some user input...
const query = queryBuilder(texasWaterWebApps)
    .search('waco');

return search(query).then(...)
jgravois commented 5 years ago

i think this is a great idea.

tomwayson commented 5 years ago

I like this idea.

I think it will be a bit challenging in TS. I wrote a fluent API for cedar and it was a lot of overloads juggling.

Also, I do think there could be a little cognitive dissonance if most of the fns in the lib are stateless fns that return promises, and then there are a few fluent (stateful) builders. I wonder how this will interplay w/ #339

All that said, I think this will provide a valuable benefit to consumers of this library.

patrickarlt commented 5 years ago

@tomwayson @dbouwman I'm really interested in writing at least the first one of these for building a search query parameter after running through the horror of https://github.com/Esri/arcgis-rest-js/blob/master/demos/node-cli-item-management/index.js#L101-L132.

Before I get too far into this though I know you both are (particularly @dbouwman) are huge FP fans. Fluent interfaces in TS would be super easy with classes but in keeping without goal of ideally NOT using classes and holding state in them I did some extra research.

  1. Something like https://hackernoon.com/writing-fluent-functional-code-384111431895 would be super cool but cant work without variadic types in TypeScript which look like their own special hell.
  2. Having builders be more factory oriented

    
    function QueryBuilder() {
      var params = { q: "" , num: 10, start: 0};
      return {
        // the type for this is actually optional here but it might add some type safety
        andTags(this: ReturnType<typeof QueryBuilder>, ...tags) {
          params.q += tags.join(",");
          return this;
        },
        toParams() {
          return params;
        }
      };
    }
    
    const params = QueryBuilder()
      .andTags("foo", "bar")
      .andTags("baz")
      .toParams();
    
    console.log(params);
    ```;
  3. We could always use classes:

    class QueryBuilder {
      private q: string = "";
      private num: number = 10;
      private start: number = 0;
    
      andTags(...tags) {
        this.q += tags.join(",");
        return this;
      }
    
      toParams() {
        return {
          q: this.q,
          num: this.num, // fetch from internal state
          start: this.start, // fetch from internal state
        };
    
      }
    }
    
    const params = new QueryBuilder()
      .andTags("foo", "bar")
      .andTags("baz")
      .toParams();
    
    console.log(params);

I'm really leaning towards option 2 here. I think it will be the easiest option.

In either case when request runs I think we should check to see if the value of requestOptions.params has a toParams() method. If it does we will call it and set the request params as the result.

To allow the builder not to have to warp EVERY param on the API (although ideally they should) we could allow an escape hatch in toParams to allow any additional params to be merged in.

function QueryBuilder() {
  var params = { q: "" , num: 10, start: 0};
  return {
    // the type for this is actually optional here but it might add some type safety
    andTags(this: ReturnType<typeof QueryBuilder>, ...tags) {
      params.q += tags.join(",");
      return this;
    },
    toParams(additionalParams) {
      return {...params, ...additionalParams};
    }
  };
}

const params = QueryBuilder()
  .andTags("foo", "bar")
  .andTags("baz")
  .toParams({someOtherParamThatIsntWrapped: true});

console.log(params);
jgravois commented 5 years ago

this will be included in v2.0.0. hopefully later today!

patrickarlt commented 5 years ago

Reopening this to continue to track discussion of additional builders.