Skip to content
This repository has been archived by the owner on Nov 25, 2020. It is now read-only.

Refactor .find() #127

Closed
RWOverdijk opened this issue Jun 20, 2016 · 11 comments
Closed

Refactor .find() #127

RWOverdijk opened this issue Jun 20, 2016 · 11 comments

Comments

@RWOverdijk
Copy link
Member

#119 (comment)

Refactor find to accept path and criteria (relative to resource).

@RWOverdijk
Copy link
Member Author

@Nexbit Hello again :) Looking at the code, I don't think this makes sense. .find() is a public method used to fetch (find) data for a specific resource. If (for instance) you want to get the delegates you have a couple options:

Association:

entity.find('user').then(user => {
  // you can use `user.delegates` if you set up the association in the entity
});

Repository:

class MyRepository extends Repository {
  findDelegates() {
    return this.findPath(`${this.resource}/delegates`);
  }
}

The repository way is a custom implementation. This has been kept open to allow flexibility. Therefor, I'd say no to refactoring find() (which has a very specific purpose).

Something like this could work, but is a breaking change:

findPath(path, criteria, raw) {
  if (path[0] !== '/') {
    path = `${this.resource}/${path}`;
  }
}

This follows the convention of absolute and relative paths. I'd need a vote on this though.

@pfurini
Copy link

pfurini commented Jun 21, 2016

@RWOverdijk I agree with your reasoning, and maybe the findPath refactoring you suggest could be added in the next major version.

@doktordirk
Copy link
Contributor

doktordirk commented Jul 8, 2016

just to make sure: you do know that criteria is used as part of the path if it is a number or string only?

so, find(1) translates to eg api/users/1
or findPath('users',1) to api/users/1

@pfurini
Copy link

pfurini commented Jul 9, 2016

@doktordirk Yeah I used that to retrieve single resources directly, but my original issue was with findPath when retrieving a related resource like orders/1/items, as described in the comment I pointed out in my first post.
I know that libraries like sails or loopback are more plain, given that they are auto-generated for the most part, but other hand-crafted apis can make good use of sub-resources (without abusing it, or it will result in a nesting nightmare).

@doktordirk
Copy link
Contributor

so you.d need a entity.find then?

@gregoryagu
Copy link
Contributor

gregoryagu commented Aug 21, 2016

I think it would be nice to have a repository method similar to .findPath(), but .call(). Same parameters, but it would call an action on top of the root path.

In other words a User repository would call "user/list/1" for .call("list",1).

I really need this for ASPNetCore where the Controller is "user" but it typically takes an action name as well.

call(path:string, criteria:any) {

        let fullPath = this.getResource() + "/" + path;

        return this.findPath(fullPath, criteria);
    }

I think this is the same issue that @Nexbit was having.

@RWOverdijk
Copy link
Member Author

If it does exactly the same, you can just extend it in your application.

@gregoryagu
Copy link
Contributor

gregoryagu commented Aug 22, 2016

@RWOverdijk Ok. I actually tried to extend it, but I am missing something in how I set it up.

I Subclassed Repository to AppRespository.

Then then set it as the Repository using the decorator on the Contact Entity Type.

import {AppRepository} from 'repository/app-repository';
@repository(CustomRepository)
...

But then when I attempted to get an instance in the viewmodel:

entityManager.getRepository('contact');

It returned a regular repository, not an AppRepository so I could call my custom methods.

Am I doing something wrong in the wireup?

@RWOverdijk
Copy link
Member Author

@gregoryagu Did you specify the @resource on the entity as 'contact'?

Also, it should be possible to overwrite the default repository. Simply register your custom repository as Repository on DI.

@RWOverdijk
Copy link
Member Author

Closing this due to inactivity. Feel free to reopen if this question still exists :)

@gregoryagu
Copy link
Contributor

Yes, this was resolved. Sorry, forgot to update this. Thanks very much.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants