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

Allow change of primaryKey and use parameters follwing JSON API specification #9

Closed
wants to merge 2 commits into from

Conversation

prodigy7
Copy link

primaryKey: See #8

parameters: Current version doesn't use compliant parameters for json paging. I modified code so far vue-api-query follows the specification.

@robsontenorio
Copy link
Owner

robsontenorio commented May 16, 2018

@prodigy7 tests have been failed :(

Before developing make sure to run yarn test then press 'a' to run all all tests. This way, on each modification on source code will re-run all tests automatically.

About pagination, check this out:
https://github.com/robsontenorio/vue-api-query#pagination

@prodigy7
Copy link
Author

Okay, thanks! The reason tests fail currently is, that the test are not adapted to the changes, I've made to pagination. The sample for pagination I know, the problem is that the parameter names doesn't follow the suggested specifications. I use https://github.com/spatie/laravel-json-api-paginate in a project which implements specification for json pagination. I think, it isn't bad following a official specification but I also understand, that this changes breaks existing code maybe. Any idea/suggestion how to resolve?

@robsontenorio
Copy link
Owner

robsontenorio commented May 16, 2018

@prodigy7 Try to submit a PR to solve only a specific feature. One for "custom primary key" and another for "pagination". Your PR will be valid if it pass on tests. If not, it is a breaking change. I recommend you focus it first on "custom primary key" feature.

@robsontenorio
Copy link
Owner

Version 0.6.0 released! README updated.

Just update the dependencies.

yarn add vue-api-query

@prodigy7
Copy link
Author

Sorry, I re-open this ticket. Whats about the json api specification conform implementation? Would be nice to see it implemented in your main sources. I know, it breaks existing code. Any idea how it could implemented?

@robsontenorio
Copy link
Owner

@prodigy7 The current implementation of vue-api-query just works with default laravel paginator. Try it.

FRONTEND

// GET /users?page=1&limit=20
let users = await User        
        .page(1) 
        .limit(20)
        .get()

BACKEND

public function index(Request $request) {
       return QueryBuilder::for(User::class)
                ->paginate($request->limit);
} 

In this case, there is no need for
https://github.com/spatie/laravel-json-api-paginate

@prodigy7
Copy link
Author

prodigy7 commented Jun 4, 2018

Sorry for re-opening again - at the moment I have a lot to do, otherwise I would also react faster.

Back to topic: The point for me is, be compliant to specifications. It does not only mean the request parameters, it's also related to return. The json-api-pagination returns results like specification (http://jsonapi.org/examples/#pagination) want see it. Currently I develope a backend with many api calls and I expect, that many different clients use that api. So it made sense, return results in a common expected format.
It is not important for me, support exactly that package. It is important for me, following the specification so many different clients can use the api without doing many "special work". That the package does implement exactly the specification is nice, but the specification it self is the "target" :)

So: Is there any way, do maybe a two way compatibility? Maybe a wrapper, supports specification or something else?

@prodigy7
Copy link
Author

prodigy7 commented Jun 4, 2018

Maybe a idea: Extending the existing class? https://stackoverflow.com/questions/15192722/javascript-extending-class
Class without extending has the current behaviour, with extending would match the specification.

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.

2 participants