Fix for $find to use a constructor on the result

[rossity]

Fixes the result of find not being able to use methods such as save because it wasn't using a constructor.

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master    #67   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         262    262           
  Branches       48     48           
=====================================
  Hits          262    262

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...4ab5f2f. Read the comment docs.

[taai]

I have done the same, but by overwriting the method in my base model. There should be a way to define, if you are wrapping your data in "data" or not. Also separately – when fetching collection and when fetching single record. And when your data is converted into a model, all the data that was not wrapped into "data", should be accessible too, probably by defining "$meta" getter. Just my toughs about this...

[robsontenorio]

@rossity Please, could you commit on PR the missing test case related to this issue?

[rossity]

@robsontenorio sorry it may be a while, too much on my plate at the moment.

@taai I completely agree, right now I'm not able to use $get() methods because it removes my pagination and other meta data that I need to reference. It would be fantastic if I could use $get() and still somehow access the resources meta data that came back outside the data attribute from the api.

[robsontenorio]

If you need metada you should to use get() instead $get() Em qua, 1 de mai de 2019 às 10:54, Ross <[email protected]> escreveu:

@robsontenorio <https://github.com/robsontenorio> sorry it may be a while, too much on my plate at the moment. @taai <https://github.com/taai> I completely agree, right now I'm not able to use $get() methods because it removes my pagination and other meta data that I need to reference. It would be fantastic if I could use $get() and still somehow access the resources meta data that came back outside the data attribute from the api. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA5BK5M36DXQJG25UKTK2TPTGOILANCNFSM4HJFDDJQ> .

-- — Robson Tenório

[rossity]

Right that is what I'm using, but the $get() method is very nice because I can just directly assign it as a model instead of having to go through data. The way it currently works is just fine, I'm just agreeing with @taai that it would be really nice for the $get() method to add a meta property to the resource model.

So you could do something like this

this.posts = await Post.$get()

// $meta() retrieves any metadata that came with the request

this.currentPage = this.posts.$meta().current_page

[robsontenorio]

Are you using Laravel in backend? If not, what response do you get from backend? Em qua, 1 de mai de 2019 às 11:17, Ross <[email protected]> escreveu:

…

Right that is what I'm using, but the $get() method is very nice because I can just directly assign it as a model instead of having to go through data. The way it currently works is just fine, I'm just agreeing with @taai <https://github.com/taai> that it would be really nice for the $get() method to add a meta property to the resource model. So you could do something like this this.posts = await Post.$get() // $meta() retrieves any metadata that came with the request this.currentPage = this.posts.$meta().current_page — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA5BK375GSYGERJO3JTJNLPTGRBFANCNFSM4HJFDDJQ> .

-- Robson Tenório

[leeovery]

I think some basic config would be a good idea.

I’ll try to find some time for a PR.

I also like the idea of getting access to the meta data too.

[robsontenorio]

If backend responds like this...

// sample of response  for paginate
{
   "data": [{"items_here"}],
   "total": 50,
   "per_page": 15,
   "current_page": 1,
   "last_page": 4,
   "first_page_url": "http://laravel.app?page=1",
   "last_page_url": "http://laravel.app?page=4",
   "next_page_url": "http://laravel.app?page=2",
   "prev_page_url": null,
   "path": "http://laravel.app",
   "from": 1,
   "to": 15
}

You should use .get() , then this.users will hold full response from backend. So, if you need to iterate over items just use this.users.data

this.users = await User
          .page(1)
          .limit(10)
          .get()

[rossity]

Yes I am and Laravel and others typically return resource responses like this

// sample of response  for paginate
{
   "data": [{"items_here"}],
   "meta": {
     "total": 50,
     "per_page": 15,
     "current_page": 1,
     "last_page": 4,
     "first_page_url": "http://laravel.app?page=1",
     "last_page_url": "http://laravel.app?page=4",
     "next_page_url": "http://laravel.app?page=2",
     "prev_page_url": null,
     "path": "http://laravel.app",
     "from": 1,
     "to": 15,
     "other_custom_meta_data": []
  }
}

so meta could be stored as a property on the resource containing the above info

[robsontenorio]

Laravel doesnt return "meta" attribute (https://laravel.com/docs/5.8/pagination#displaying-pagination-results). By the way, we can't figure out what kind of format the backend will respond. So, get() is the best option to get entire response as described on my last comment.

[rossity]

Laravel does if you're using Laravel API resources https://laravel.com/docs/5.8/eloquent-resources#writing-resources (scroll down to pagination)

The other most popular api resource formatter Fractal does the same https://fractal.thephpleague.com/pagination/

And that's fine, I understand that you want this library to be pretty agnostic to backend responses. I just found this library through spatie's query builder which uses the above standard for pagination/meta data.

[jaumesala]

Yes! I'm in the same situation. @rossity check my PR #68 As @leeovery says "I think some basic config would be a good idea." In my PR I have added a method dataWrappers() to configure how the data is returned from the API. It's a WIP but I'm trying to solve this problems.

[leeovery]

Im a bit confused at the need for this now I look at it. Im doing this fine...

[robsontenorio]

@jaumesala @rossity @leeovery I think we dont need "extra code" for any king of pagination. If backend responds with:

{
    "data": [
        {
            "id": 1,
            "name": "Eladio Schroeder Sr.",
            "email": "[email protected]",
        },
        {
            "id": 2,
            "name": "Liliana Mayert",
            "email": "[email protected]",
        }
    ],
    "links":{
        "first": "http://example.com/pagination?page=1",
        "last": "http://example.com/pagination?page=1",
        "prev": null,
        "next": null
    },
    "meta":{
        "current_page": 1,
        "from": 1,
        "last_page": 1,
        "path": "http://example.com/pagination",
        "per_page": 15,
        "to": 10,
        "total": 10
    }
}

When you use get() you are totally fine. Can someone point me if i am wrong?

[leeovery]

I agree

[rossity]

Yeah I think you are right, it's fine as is. But the original PR is a needed change! We definitely got off topic. 😋

[leeovery]

Im confused why this change is needed. The promise has a thenable attached via the original find method which does the work of wrapping the returned data into the constructor etc.

You change makes it happen twice.

[rossity]

Because in the original find the constructor is on the response and $find() just picks the data attribute out of the constructed model, which means that the result of $find() itself is not actually a constructed model.

[leeovery]

Ah!! Im with you now. @rossity

@robsontenorio This is deffo needed. My bad!!

[robsontenorio]

Released 1.5.1 Thanks @rossity !