christianmalek / vuex-rest-api

A utility to simplify the use of REST APIs with Vuex
http://vuex-rest-api.org
MIT License
382 stars 48 forks source link

Improvement ideas #54

Closed christianmalek closed 4 years ago

christianmalek commented 6 years ago

I'm willing to improve Vapi. Any ideas?

Things I'm planning to do are:

I would love to hear ideas from everyone who is interested in this project. :)

slurmulon commented 6 years ago

I love the library, it has a clean interface and does an excellent job!

What do you think about supporting custom pre-flight validation methods on the non-idempotent HTTP methods (e.g. POST, PUT, PATCH)? Requests would only be sent out over the network if the provided request body passes the user's validation function, otherwise a custom error callback could be invoked.

One use case would be allowing easy integration with tools like JSON Schema. Users could just provide a callback that passes the desired request body and a JSON Schema corresponding with the API resource into a validation module such as ajv or is-my-json-valid.

christianmalek commented 6 years ago

Hello @slurmulon , custom pre-flight validation methods sound indeed useful and the integration with JSON Schema is also a neat idea I've never thought about. Thanks for your contribution and the compliment!

At the moment I'm waiting for changes on Vuex. Seems like there will be several big changes in the next major release.

webstractions commented 6 years ago

First, this is a sweet resource. I have just stumbled upon it.

It would be great if the endpoints can be mapped via a configuration file, rather than chaining a bunch of .get, .post, etc methods to the new class (or .add this, .add thats).

Take an API such as TMDB with scores of endpoints, and it can get out of hand and hard to read. There are a couple of javascript TMDB repos that handle this via an endpoints.js file that they use to generate the actual API routes from. See impronunciable/moviedb and grantholle/moviedb-promise.

Having a config file would allow one to easily swap out API's, let's say I want to use TRAKT for media info. I can create another endpoint file and keep the same action names within vuex, just using a different API to get the data.

christianmalek commented 6 years ago

@webstractions This seems also plausible to me. I've added it on the list of features for v3 (see #57). Thanks for your contribution!

webstractions commented 6 years ago

@christianmalek I am working on something right now using this repo. Chances are, I will be writing a function to import the endpoint mapping and generate the gets, posts, etc via an .add loop of some sort. If you want, I can fork this and submit a PR for you.

christianmalek commented 6 years ago

@webstractions PRs are always welcome. Please note that the base of this utility is written in TypeScript. So I will only accept PRs in TypeScript.

webstractions commented 6 years ago

@christianmalek Yikes. I just noticed that. Looks like I need to pick up a book and do a little brushing up.

someone1 commented 6 years ago

Maybe a CLI tool for parsing a swagger/OpenAPI spec file to an (typescript) output leveraging vuex-rest-api? Or a load function that takes a swagger spec and spits out a Vapi object?

Not sure how, if at all, this fits into this project. I can already generate a typescript client using swagger-codegen which does a LOT of the work for me, but I can't plug that into vuex without the manual boilerplate. This project would alleviate that but basically recreates the API client for me, but I can't generate the client.

christianmalek commented 6 years ago

@someone1 This sounds good to me at first glance. But I think this will exceed the scope of vuex-rest-api because:

slurmulon commented 6 years ago

What about a global onError interceptor that can be overridden at the resource entity level?

I imagine this might be tricky to achieve given module namespacing in vuex, but I think it would be very useful - that way you don't have to write/import the same onError snippet in every single resource entity.

christianmalek commented 6 years ago

@slurmulon Seems reasonable. I've added it to the list!

christianmalek commented 6 years ago

What do you guys think about an alternative syntax of adding the actions?

Here are some ideas:

.get({
  action: "getPost",
  property: "post",
  path: ({ id }) => `/posts/${id}`,
  // additional params...
})
.get("getPost", "post", ({ id }) => `/posts/${id}`, {
  // additional params...
})
.add("GET posts/:id INTO post AS getPost", {
  // additional params...
})
.get("posts/:id INTO post AS getPost", {
  // additional params
})
slurmulon commented 6 years ago

@christianmalek I think that the configuration object approach is still the best.

For what it's worth, here's my thinking (skipping over the last idea since it's so similar to the idea before it):

Idea 1

.get("getPost", "post", ({ id }) => `/posts/${id}`, {
  // additional params...
})

This approach is regressing back to ES5 days where we didn't have parameter destructuring and therefore it misses out on the benefits.

The primary downside is that the user has to remember the ordering of the parameters. It also makes omitting arguments awkward (e.g. you have to use null to skip over an unneeded param).

Idea 2

.add("GET posts/:id INTO post AS getPost", {
  // additional params...
})

To me the parameters in this context are so straight forward that they don't justify creating a special syntax.

It also loses out on the benefits of having path as a Function, so in certain situations you will end up having to use some config object anyways.

For example, what if id is a nested property? The only way I can see it working is by adding some extra params configuration with a callback or by using something like JSON Pointer (like what JSON Hyper-Schema does)

.add("GET posts/:id INTO post AS getPost", {
  params: {
     id: entity => entity.data.id // or `'#/data/id'`
  }
})

You could also inline JSON Pointer, but at that point it's getting kind of complicated and still isn't as powerful (or pretty) as a simple callback:

.add("GET posts/{#/data/id} INTO post AS getPost")

In general it feels strange that some functionality is covered by this specially formatted String but other functionality is not.

I think an idiomatic and homogeneous solution (like what exists today) is simply more intuitive and less complex than what's being proposed.

BorisTB commented 6 years ago

Here's my idea to new syntax with some new functions I miss on current version with examples how they could be used:

.actions({
  getPost: {
    method: 'GET',
    path: ({ id }) => '/post/${id}',
    property: 'post'
  },
  getPosts: {
    pluralOf: 'getPost'
  }
})
.common({
  onSuccess: (actionConfig) => (state, payload, axios) => {
    const { data, metadata } = payload.data
    state[actionConfig.property] = data

    if (metadata) {
      state[actionConfig.property + 'Metadata'] = metadata
    }
  },
  retry: {
    delays: (attempt) => Math.pow(2, attempt + 4) * 100,
    timeout: 15000,
    statusCodes: (statusCode) => statusCode >= 500,
    beforeRetry: ({ forceRetry, abort, delay, attempt, lastError, req }) => {
      if (attempt > 10) {
        abort()
      }
      window.forceRetry = forceRetry
    },
  }
})
  1. .actions() method to define actions
  2. Defining by action names in same object would prevent duplicates in action names and users could easily create some helpers to build their config from some array of endpoints definitions (e.g.: const actions = endpoints.reduce((acc, item) => ({ ...acc, [item.method + item.entityName]: { /* ... */ } }), {}))
  3. method prop would define method type, which is pretty boring, but it would be cool if it could automatically get method type from action's name if not defined by user (actionName.startsWith('get') = GET, startsWith('update') = PUT, ...)
  4. Plural helper would automatically create plural action of other action, which means it would just copy path without params and property of that action and add 's' to the end of both. e.g.: if action getPosts has setting pluralOf: 'getPost' it would set path: '/posts', property: 'posts' to getPosts action. Of course the problem here is how to deal with id or other path params.
  5. .common() method to define common properties for all actions in a module (or also in all modules somehow). Functions defined in common method would be wrapped in another function that will provide config of an action, that triggered this function. e.g. action getPost triggered onSuccess, that is defined as common, vuex-rest-api would call something like common.onSuccess(actions.getPost)(state, payload, axios, actionParams)
  6. retry property is inspired by fantastic retryMiddleware from react-relay-network-modern. It would help user to retry request if error occured. Also it would probably be useful to send retry function as argument to onError callback delays: number, array of numbers or function that receives number of attempt as argument to calculate delay between retry requests timeout: number that represent max wait time for response statusCodes: array of numbers or function defining which response status codes will trigger retry beforeRetry: function called before retry attempt, enabling user to stop another retry attempts or edit retry request before sending
christianmalek commented 6 years ago

@BorisTB Thanks. These are some awesome ideas. Thanks for your input!

ad 1: I'm with you. ad 2: Awesome idea. The only downside would be the API incompatibility. ad 3: Here I'm also with you. I would say that raising an exception in case of unknown action commands (get, post, put) would be a good addition to this (in favor of just defaulting to eg GET). ad 4: The idea of an plural helper sounds nice. But I don't think this is the proper way. Maybe we should open another issue for this and discuss the pros and cons of different implementations. ad 5: Something like this is planned. I think it would be clever to discuss pros and cons of this first before implementation. ad 6: I like this. Do you know a good library for this?

qskousen commented 5 years ago

Some way to cancel a current request would be fantastic. In my use case, I have a web page that automatically refreshes the store every X seconds. You can also do a search or a filter and it will refresh. Sometimes, the search and the auto-refresh happen near the same time, and you will get a flash of your search and then it will be overwritten by the auto refresh, which was sent out first (before filters changed) but came back last, overwriting the filtered results. Then the next auto refresh will correctly have the filtered results... All in all, a very confusing user experience.

I looked into ways of cancelling a current request and found this: https://stackoverflow.com/questions/30233302/promise-is-it-possible-to-force-cancel-a-promise#30235261

The proposal itself has died, but the token method does work. Perhaps there is a way to work this in?

A really awesome way to implement this would maybe be a property on a configuration that you can set to automatically cancel any older requests when you send out a new one, so we can ensure we always are applying only the latest request. Ability to override the setting per-dispatch for extra points.

christianmalek commented 5 years ago

Hey @qskousen,

Thanks for your suggestions! What would be your preferred way to cancel requests if you had to do it manually?

I looked into ways of cancelling a current request and found this: https://stackoverflow.com/questions/30233302/promise-is-it-possible-to-force-cancel-a-promise#30235261

axios supports already the proposed cancellation technique: https://github.com/axios/axios#cancellation But it's not built into vuex-rest-api yet. Noted for version 3.

A really awesome way to implement this would maybe be a property on a configuration that you can set to automatically cancel any older requests when you send out a new one, so we can ensure we always are applying only the latest request. Ability to override the setting per-dispatch for extra points.

Sounds good in my ears. I've noted it for version 3, too.

qskousen commented 5 years ago

@christianmalek, If I was to work within current vuex-rest-api to cancel a request manually, I would be overriding the onSuccess and having a token (similar to described in the SO answer) that would tell me at that point if I needed to commit the response, or ignore it because I have a newer one pending. It would basically be a wrapper around vuex-rest-api to handle the cancellation process.

I am not 100% sure if that was the question you were asking - if not, let me know! Thanks.