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

data wrapping, model hydratation and access to response instance #68

Closed

Conversation

jaumesala
Copy link

@jaumesala jaumesala commented Apr 30, 2019

This pull request is to handle request to JSON API’s with and without data wrapping.
Basically you only need to configure your model and define how your data is wrapper in each response operation.

Configration

You only need to define de value of dataWrappers()
By default, all models operate without data wrapping, for backward compatibility.

  dataWrappers() {
    return {
      index: null,
      store: null,
      show: null,
      update: null,
      destroy: null,
    }
  }

So, if your API returns all your data inside a data attribute, then you should set up dataWrappers() like this:

  dataWrappers() {
    return {
      index: 'data',
      store: 'data',
      show: 'data',
      update: 'data',
      destroy: 'data',
    }
  }

In case your API only wraps the resources collections, then, you only need to set up that response type:

 dataWrappers() {
   return {
     index: 'data',
     store: null,
     show: null,
     update: null,
     destroy: null,
   }
 }

Well, you get the idea...

Model Instance hydration

Another thing this pull request solves is the model hydratation. All models will be hydratated after each CRUD operation. This is usefull if your API returns a full representation of the resource. This avoids a second request to get the new values.

 // resource before request
 // {
 //   id: 1,
 //   username: 'arya'
 //   email: 'arya.stark@winterfell.com'
 //   token: '1234'
 // }

 // PUT /users/1
 let user = new User({ id: 1, email: 'arya@sevenkingdoms.com' })
 await user.save()

 // response from API for PUT /users/1
 // {
 //   data: {
 //     id: 1,
 //     username: 'arya'
 //     email: 'arya@sevenkingdoms.com'
 //     token: '1234'
 //   }
 // }

 user = {
   id: 1,
   username: 'arya'
   email: 'arya@sevenkingdoms.com'
   token: '1234'
 }

This is very useful even for a DELETE request, if you return the deleted values.

Access to response instance on save.

Finally, now, method save() returns the response instance unmodified. The models are stored in the instance itself (hydrated), but the returned value is the http response. Again, this is useful if we want to access metadata information in the response data.

  let user = new User({name: 'Arya'})
  res = user.save()

  // "user" is the User Instance
  user = {
    id: 1,
    name: 'arya',
  }

  // "res" is the http response
  res = {  
    data: {},
    status: 200,
    statusText: 'OK',
    headers: {},
    config: {},
    request: {}  
  }

Tests...

I've added test for all the cases (I think). By the way, I added some "deprecated" comments (in tests) for methods $get(), $first(), $find() as I think those methods are redundant now? I don't know, what do you think?

@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #68   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         262    259    -3     
  Branches       48     47    -1     
=====================================
- Hits          262    259    -3
Impacted Files Coverage Δ
src/Model.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 246af42...a7e6182. Read the comment docs.

@robsontenorio
Copy link
Owner

@jaumesala Did you try latest release? It provide new "fetch style" methods like $find() and $first(), besides $get().

For instance, in custom scenarios you can decide, if you will use find() or $find(), first() or $first(), get() or $get(). Because, you already know what to expect from backend.

@jaumesala
Copy link
Author

@robsontenorio yes, of course. I'm using your package in a project I'm working on. In my case my API endpoints return the responses inside data wrappers. This cause me to do extra work on CRUD operations. My patch is to unify and simplify the use of those methods as much as possible.

In example: If I update a model, the API returns the updated values inside a data wrapper. Without this patch my model ends with a Model.data property which contains all the values.

// A PUT request to /posts/1 returns the updated values inside a "data" property 
post = {
  id: 1,
  title: 'hello',
  data: {
    id: 1,
    title: 'world'
  }
}

Then Object.assign() updates the model instance and then I have to do extra work....

Before coding this patch, I checked open and closed issues and I found more people where in the same situation as me. In fact I think the reason $methods() exist is to try to solve this situations, as far I can understand. #55 #61 #67 ...

I tried to add as many tests as possible to cover all possible scenarios. And it seems to work. So even people already using the package should not have any problem.

Let me know if you need more information.

@jaumesala
Copy link
Author

Now that I had 5 minutes, I checked the code and I think I found a not covered case. When the get() method returns more than one base property like this:

// response to GET /posts with data wrapping and pagination
{
  data: [
    { /* posts */ }
  ],
  links: {},
  meta: {}
}

With the current patch you cannot acces the links and meta data.
If you don't mind, hold on to this PR. More commits may be comming.

@robsontenorio
Copy link
Owner

@jaumesala Be aware this package does not intend to be full compliant with JSON API spec. If you need it so, please check some options here before go further.

https://github.com/robsontenorio/vue-api-query#thanks

@jaumesala
Copy link
Author

@robsontenorio Totally agree, I just want to make it is able to handle data inside a wrapper and ensure all methods allows you to access the response object. That's alll, the rest of the logic is up to the developer.

@leeovery
Copy link
Contributor

leeovery commented May 2, 2019

Ive taken a look at this and Im not convinced we need the config for the data wrapper. The fetch style methods are working fine for me. I return all my data wrapped in data and am handling it fine, along with getting access to the meta.

Something that might be useful is getting access to the meta while still using those fetch-style methods. Thats something we cant currently do. As I need access to the meta property Ive reverted to using get (as opposed to $get).

Thoughts @robsontenorio

@jaumesala
Copy link
Author

@leeovery my point is, instead of using get() or $get() // first() or $first() // find() or $find, just use get(), first() and find(). And, as you says getting access to the meta data. check this situation:

let user = new User({name: 'Arya'})
res = user.save() // POST /users

// "user" is the User Instance
// with its data hydrated from the response.data(.data)
user = {
  id: 1,
  name: 'arya',
}

// "res" is the http response
// we can access its "meta", "links"
res = {  
  data: {
    data:{
      id: 1,
      name: 'arya',
    },
    meta: {
      message: "hello!"
    },
    links: {}
  },
  status: 200,
  statusText: 'OK',
  headers: {},
  config: {},
  request: {}  
}

In the PR I submited the other day I didn't pay much attention to the difference between a fetch() and $fetch() method. I just made the test pass while adding the new code. Then I realised there's a problem in my code (I will fix it soon).
The reason for my PR is your previous comment "getting access to the meta while still using those fetch-style methods." in ALL situations:

  • first()
    Static call, returns http response object
  • find()
    Static call, returns http response object
  • get()
    Static call, returns http response object
  • save() [_create()]
    Hydrate model instance (example above) and returns http response object
  • save() [_update()]
    Hydrate model instance and returns http response object
  • delete()
    (Hydrate model instance?) and returns http response object

What do you think?

@leeovery
Copy link
Contributor

leeovery commented May 2, 2019

Its not my call anyway... but....

Im not convinced with the config for whether an item is data wrapped or not.

I think the fetch style methods ($find, $get etc) are there to return the data from the response, and leave behind the rest. Thats why those methods exist.

Im also not entirely convinced at returning the full response object. I kinda like how it gets stripped off and I don't have to care about it.

When I use $get() I expect the hydrated models and nothing else. When I use get() I want the data property. It all depends on how your backend packages up your responses ofc but thats how I do it :)

Having said these things.... I wonder if we could expose an api to get access to the previous $meta and $response. Then for those that need it they can get to it.

So, given a response object like:

res = {  
  data: {
    data:{
      id: 1,
      name: 'arya',
    },
    meta: {
      message: "hello!"
    },
    links: {}
  },
  status: 200,
  statusText: 'OK',
  headers: {},
  config: {},
  request: {}  
}

Would result in being able to do:

const user = User.$get() || User.$find(1) || User.$first()

user === User: { id: 1, name: 'arya' } (hydrated User model)
User.$meta() ===  { message: "hello!" }
User.$response === res (untouched as received from backend)

Being able to access the meta would be beneficial and would enable user to use fetch style methods alongside being able to get meta.

@robsontenorio
Copy link
Owner

@jaumesala i agree with @leeovery it can be solved with current codebase. I think the best approach is open separate issues. So we can dig one problem at time.

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

Successfully merging this pull request may close these issues.

None yet

3 participants