Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Param builders for building complex parameter objects #384

Open
patrickarlt opened this issue Nov 8, 2018 · 6 comments
Open

Param builders for building complex parameter objects #384

patrickarlt opened this issue Nov 8, 2018 · 6 comments
Assignees

Comments

@patrickarlt
Copy link
Contributor

Given that this library now has the capacity to do routing #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
Copy link
Member

dbouwman commented Nov 8, 2018

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
Copy link
Contributor

jgravois commented Nov 9, 2018

i think this is a great idea.

@tomwayson
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@patrickarlt
Copy link
Contributor Author

Reopening this to continue to track discussion of additional builders.

@patrickarlt patrickarlt reopened this May 1, 2019
@jgravois jgravois removed their assignment May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants