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

Add JSON API (v1.0) compliant API option #396

Closed
Jeehut opened this issue Oct 4, 2015 · 11 comments
Closed

Add JSON API (v1.0) compliant API option #396

Jeehut opened this issue Oct 4, 2015 · 11 comments

Comments

@Jeehut
Copy link

Jeehut commented Oct 4, 2015

With the final specification of the great JSON API ready and this gem providing a very good solution to authenticating via APIs I find it natural to expect this gem to comply to the new JSON API standard sometime in the near future.

As an example I am currently writing a new API based on the jsonapi-resources gem where I am also using devise_token_auth for higher security measures. But mimicking all the functionality within this gem via my own controller/routes/etc. in order to comply to the JSON API standard is not only a lot of work but also bad in terms of future changes.

As I believe though that many more people will want to comply to the JSON API standard in the future (due to time savings because there a lot of implementations ready to go) I am asking the maintainers here if they are interested in adding an option to comply to that standard. Specifically I would love to help building that option but before I do so I'd love to know the chances of such a pull request getting merged and maybe a hint from the maintainers where/how they would start implementing the feature in a clean way.

Any inputs / opinions / ideas on this?

@Jeehut Jeehut changed the title Feature request: Add JSON API (v1.0) compliant API option Add JSON API (v1.0) compliant API option Oct 4, 2015
@lynndylanhurley
Copy link
Owner

@Dschee - this seems reasonable. Is this something that you would like to take on?

@Jeehut
Copy link
Author

Jeehut commented Oct 13, 2015

@lynndylanhurley - Yes, indeed, I'd love to take it on but I can't right now. Will have a look at it sometime later. Until then it would be great to get some hints on where to start it and/or how to include it so I don't interfere with any features of this great gem so people can still use it without the JSON API addition. Any thoughts on that?

@lynndylanhurley
Copy link
Owner

This should be straightforward actually. Most or all of the controllers have the render methods broken out (example), so it will just be a matter of formatting the responses to match the spec.

The problem is that this will break compatibility with ng-token-auth and j-toker, so those libraries will need to be updated as well.

And updating the tests will be kind of a pain.

But I've always felt that the responses should follow a standard format, and the new JSON API spec looks nice.

@booleanbetrayal - do you have any thoughts on this?

@booleanbetrayal
Copy link
Collaborator

As long as the JSON-API spec is final, it's an optional / backwards-compatible feature (initially), docs are comprehensive, and spec coverage is good, I think I'm fine with it. =]

I don't have a lot of free-time to offer these days, but I would gladly spend some time with a good PR.

@Jeehut
Copy link
Author

Jeehut commented Mar 8, 2016

Hey guys, sorry for the multiple months of delay, I was just too busy all this time. Now I had some free time to start this off which I did in #566. It's not finished yet and I plan to continue my work in the weeks ahead. You can review my changes as I make progress and can give some feedback early on if anything is going the wrong direction. I hope it's going to be useful to some folks out there! :)

@Jeehut
Copy link
Author

Jeehut commented Mar 19, 2016

I've just completed the response side of the JSON API compliance. See #566. Note that #566 alone doesn't fix this issue yet (so please keep it open) – I plan to add JSON API compliant requests, too.

@zachfeldman
Copy link
Contributor

Hi there @Dschee ,

In an effort to cleanup this project and prioritize a bit, we're marking issues that haven't had any activity in a while with a "close-in-7-days" label. If we don't hear from you in about a week, we'll be closing this issue. Obviously feel free to re-open it at any time if it's the right time or this was done in error!

If you are still having the issue (especially if it's a bug report) please refer to our new Issue Template to provide some more details to help us solve it.

Hope all is well.

@Jeehut
Copy link
Author

Jeehut commented Oct 17, 2017

This close-in-7-days label really is funny. It should be renamed to ignore-2-years-until-developer-gives-up-first-then-close-in-7-days.

I'm really happy you are updating stuff in this project @zachfeldman – so please don't get me wrong – but that reminds me of the even bigger sadness of having no activity by the maintainers for a long time. I really invested quite some time after they gave me their OK that the JSON API stuff might get merged. But that work was ignored for a long time and having all of my issues and PRs closed without any contextual reason is a bitter end of the story.

@zachfeldman
Copy link
Contributor

@Dschee I'm really sorry that you feel that way, I mean it. I also have worked on open source projects before where I worked extremely hard on something only to be ignored for a very long time by the maintainers, or never even acknowledged at all.

I stumbled upon this project because my company needed to do token authentication and I personally was also frustrated by the inactivity and lack of a Standard in Rails for this (with Devise). I decided to get in contact with @lynndylanhurley to see if I could pitch in some of my free time and help clean things up and get it started again. We have merged a few PRs since then and addressed some open issues too, and are starting to spin the wheels again.

If you'd like we can reopen this issue and pull request. Just to remind you though that this is a volunteer-run project, not anybody's full-time job so just like you pitched in your time, I just took time to write this response and attempt to clean up the issue and pull request queue.
I don't want to speak for @lynndylanhurley , but from what I can tell this project was initially for his work but has now grown to be a lot more and something he may not even use at his day job day today, but he's involved still.

The alternative to the close-in-7-days label is somehow thinking we live in a world where the 380 open issues and 50 open PRs would get closed when, since opening that label and "cleaning things up", we still have 78 open issues and 14 open pull requests weeks later. My aim with the label is that I wanted to be more realistic about our chances of making an impact on issues with people that were still using this gem, so that we didn't waste time responding to people that moved on a long time ago. I definitely didn't mean to belittle your work, so I'm sorry if you felt that way. Thanks for the work you did in the past. There's your contextual reason. And sorry this comment became a novel!

@Jeehut
Copy link
Author

Jeehut commented Oct 17, 2017

Haha, thanks @zachfeldman for the answer! I appreciate it. And the label joke was not directed at you. As I said, I'm happy you're trying to clean things up here, you're doing just the right thing at this point.

I totally understand how these things work and that everything is done on a volunteer basis. I maintain some repositories myself which I don't use any more. But then, I wouldn't ask someone to put in some effort for a feature he wants and tell him I'd review and merge it once finished to ignore him from then on. That's simply irresponsible if not even rude. That's what makes me so unhappy.

@lynndylanhurley
Copy link
Owner

@Dschee - thanks for taking the time to submit a PR.

As @zachfeldman said, we're using the close-in-7-days to filter out issues that no one cares about any more. If you still want this merged, please re-open the issue and we'll look into it as soon as we can.

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

No branches or pull requests

4 participants