Refactor fetchr API

[redonkulus]

@snyamathi @lingyan @pablopalacios

Goal

Make the API more intuitive and easy to use for developers

Background

Some of this work is based on the discussion in #263 (comment)

Proposals

Simplify CRUD signatures

• Remove method chaining and just pass an options object with everything that is needed inside. It would be easy to mock and would make it clear which options are being passed (and wouldn't require unnecessary typing as the end call or the double null).
• Remove callback interface
• Remove end() method

Current:

fetchr.read('resource', params, { timeout: 500 });
fetchr.create('resource', params, body, { timeout: 500 });

Proposal:

fetchr.read({
  resource: 'foo',
  params: Object,

  // additional options
  cors?: Boolean,
  constructGetUri?: Function
  retry?: Object,
  timeout?: Number
  unsafeAllowRetry?: Boolean,
});

fetchr.create({
  resource: 'foo',
  params: Object,
  body: Object,
  
  // additional options
  cors?: Boolean,
  constructGetUri?: Function
  retry?: Object,
  timeout?: Number
  unsafeAllowRetry?: Boolean,
});

[lingyan]

I like the proposal. Some questions:

• Can you clarify what goes into data? I assume it is post/put body.
• Original API's second argument (first null) is for resource params, third argument (second null) is for post body, and last argument is for request-specific configs. Will resource params become params in the second argument of the new API?
• Are there example of other configs, besides timeout?

Would be good to add these details in the proposal.

[redonkulus]

@lingyan

I like the proposal. Some questions:

• Can you clarify what goes into data? I assume it is post/put body.

Yup, that was intended to be post body data. We could figure out what the correct property name should be (data, body, postBody, etc). body is used in the code and documentation today.

• Original API's second argument (first null) is for resource params, third argument (second null) is for post body, and last argument is for request-specific configs. Will resource params become params in the second argument of the new API?

I think that makes sense, One potential option could be something like this (which basically removes the first param and puts it into the object):

fetchr.create({ resource: 'foo', body: {}, params: {}, timeout: 500 });

NOTE: I don't like having timeout where it is, see comment below.

• Are there example of other configs, besides timeout?

We have them in the readme, the config that gets passed into the clientConfig method. Examples include timeout, retry, unsafeAllowRetry, cors, constructGetUri. So we may need to contain these in a config property or something more meaningful like contextConfig or requestConfig. Not sure on the name but something to make it clear its different from params and its per request.

Would be good to add these details in the proposal.

[pablopalacios]

I like the idea of moving everything to just one object. In the future, the resource property could also accept an array of resources so we could trigger many requests in the server. This is something that would be trickier to do with the current API.

I see no problem, however, on keeping timeout and etc flattened in the same object. I actually prefer that since it would simplify the process of merging the global configuration and the per request configuration. It would also simplify the updateOptions method since we wouldn't need deepmerge anymore neither come up with something such as mergeOptions in http config. Having it flat would also make it easy to set default values in case the user doesn't specify them.

From the user perspective, I would also prefer a more flat structure since I could have a module called post.js with all the fetchr methods inside, a shared configuration in the top of the module and then local configurations per method. Merging all the configs would be a breeze if everything is flat. Something like:

const baseConfig = {
    resource: 'post',
    timeout: 3000,
    retryStatusCodes: [408, 422, 502]
} 

const fetchPosts = (params) => {
    const config = { ...baseConfig, params, timeout: 5000, retryAttempts: 2 }
    return fetchr.read(config).then().catch()
}

const createPost = (body) => {
    const config = { ...baseConfig, body }
    return fetchr.create(config).then().catch()
}

[redonkulus]

I'm fine with this approach too, we could flatten it too and just have params stay the same and be for the resource parameters. @lingyan @snyamathi thoughts?

[pablopalacios]

By the way, these are all the options that we "support":

Global options

const fetchr = new Fetchr({
    corsPath, // string, path to be used in case clientConfig.cors is true
    xhrPath: options.xhrPath, // string, path to be used when clientConfig.cors is false

    headers, // object, key/value map of headers

    context, // object, that is converted to query params
    contextPicker, // string|array|function(value, key, object) to retrieve params from context

    xhrTimeout: options.xhrTimeout, // number, request timeout

    statsCollector, // function, hook to run after every request
});

Per request options

const clientConfig = {
    uri, // string, if set, overrides global cors/xhr paths
    constructGetUri, // function, to generate the final url
    id_param, // string, sets the param that is used as id, constructGetUri will set it in the url if present

    headers, // object, if set, overrrides global headers

    post_for_read, // bool, use POST for read operation

    timeout, // number, request timeout value
    xhrTimeout, // number, same as timeout

    cors, // bool, decides if it's going to use xhr or xdr path
    withCredentials, // bool, if should include cookies on cors requests
    useXDR, // bool, if it should use XDR instead of XHR (currently there is a bug that this is impossible to be set, but since xdr is dead...)

    unsafeAllowRetry, // bool, allow retry for POST requests
    retry: {
        interval, // number, interval btw attempts
        maxRetries, // number, max number of attempts
        statusCodes, // array, http status codes allowed for retries
    },
};

[redonkulus]

Mix of case style is disappointing, might make those all camelcase too.

[pablopalacios]

My proposal:

const proposal = {
    path, // string, replaces xhrPath, corsPath and uri
    urlBuilder, // function, replaces constructGetUri
    headers,
    context, // object|function, replaces contextPicker
    timeout,
    statsCollector,
    usePostOnRead, // bool, replaces post_for_read
    withCredentials,

    retryAttempts, // number, replaces retry.maxRetries
    retryInterval, // number, replaces retry.interval
    retryOnPost, // bool, replaces unsafeAllowRetry
    retryStatusCodes, // array, replaces retry.statusCodes
};

[redonkulus]

I like having xhrPath, coresPath and uri being combined, makes it simple without a ton of different options. One question is what if xhrPath and corsPath need to be different? Currently, corsPath is the origin and xhrPath is just the path:

const fetcher = new Fetcher({
    corsPath: 'http://www.foo.com',
    xhrPath: '/fooProxy',
});

[pablopalacios]

I would then rename it to url (this is the argument name in mdn for XMLHttpRequest.open).

Currently it's either one or another: https://github.com/yahoo/fetchr/blob/master/libs/fetcher.client.js#L236 and they are never concatenated. Since XMLHttpRequest supports CORS, we can pass a path or a full url and it will work.

[pablopalacios]

Besides changing the request object, I think it would be very cool if we could let the users name the operations. We could use POST by default and add an useGET option to change to GET. In this way we could make fetchr super flexible:

Fetcher.registerResource('todo', {
    readItem: function () {},
    readList: function() {},
    toggle: function() {},
    delete: function() {},
    archive: function() {}, 
});

Also, we could add a meta param to the register function with arbitrary data. This data could be used in a middleware (for example, a documentation route that lists all resources and their operations):

Fetchr.registerResource('todo', operations, {
    upstreamService: 'todo-v1',
    usesDb: false,
    shouldCache: true,
    // ...
});

[redonkulus]

I'm fine with the meta data for registerResource. I think the original reason for using the CRUD names was to have a clear API for how to use the fetcher resource. I'm not sure if I want to obfuscate that with user names. Within the code, the user can always organize the fetchr to use any number of methods, but having standard CRUD method names is good for self-documenting and usage.

[pablopalacios]

the user can always organize the fetchr to use any number of methods

@redonkulus Is this possible right now?

[redonkulus]

@pablopalacios Ya its just an implementation detail for the fetchr class.