-
Notifications
You must be signed in to change notification settings - Fork 143
Rewrite (v3.0.0) #141
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
base: master
Are you sure you want to change the base?
Rewrite (v3.0.0) #141
Conversation
42f8de9
to
6e92450
Compare
@stas So still trying to think about how this solves #21 that is flexible for very complex relationship definitions. I could see it working easily for 1 level deep relationship. However, I am still seeing potential issues of how I would compare So let's say I have a parent entity called Agent and it has a couple relationships with the same nested relationship (eg. relationship_a.team, relationship_a.team.memberships, relationship_b.team, relationship_b.team.memberships) I could possibly see a solution if the proc could derive the relationship tree structure the serializer was accessed through - see below (at which this proc could be an app specific abstraction)
So if I passed I hope that makes sense. Happy to explain more. |
@arinhouck, apologies, but I prefer to keep the conversation around #21 in it's own issue. Let's keep this PR for review purposes alone. Thank you! |
def for_object(object, mapping = nil) | ||
if mapping.is_a?(Hash) | ||
return ( | ||
mapping[object.class] || NotFoundError.new(object, mapping.values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the raise
keyword is missing here.
mapping[object.class] || NotFoundError.new(object, mapping.values) | |
mapping[object.class] || raise NotFoundError.new(object, mapping.values) |
@stas Appreciate the response That said, I don't think you and I necessarily have the same definition of maintained. You've publicly stated that you aren't taking any contributions against v2. Of any kind. Bugfixes, security updates, what have you. In my view, that's unmaintained. If the gem is still maintained, I would expect you to be taking those maintenance contributions against that version (I would've submitted the PR with CI update myself, had that message not been on the README). And that's the issue - not that the Ruby version wasn't in CI already, but that the message was that such a contribution wouldn't be welcome. The v3 is not ready for use in a production project, regardless of what my test status locally shows. It's a draft PR, there are conflicts with master, the tests all show as failing, and it's had no contributions in nine months. I'm not going to build a new project on it as is. I think most people would make the same judgement. I'm a gem maintainer myself (Dalli), and I know it can be a lot of work and that life can pull you away from it for long periods of time. So this is not a judgement on your work - I have nothing but appreciation for the time you've committed. But as I (or anyone else) decides what serialization library I'm going to use, maintainability over time is likely to be a serious consideration. For what it's worth if I were in your shoes I would:
My interest in v3 features is limited, but with a clear enough task list I'd be willing to put in some development work there. If there's any specific way you think I can help, please don't hesitate to ask. Thanks again. |
@petergoldstein I appreciate your feedback and please allow me to disagree with your suggestions! To be honest I'm not sure I understand what's your message. If you're looking for me to start accepting new stuff for the existing version, that's not going to happen (bugfixes and security updates were never refused, not sure where this is coming from) and the plan is to eventually replace the current master with this branch. TL;DR here is that we tried it before and it's not working because of the legacy decisions. Again, I'm happy to look into a particular issue related to this project or discuss any feedback regarding to this PR, but at this point I find this conversation not very helpful. |
I'm not finding it particularly productive either, largely because there seems to be a very weird assumption here. If you put out a big notice that says 'Contributions not welcome!' then you can't say 'well of course bug fixes and security updates would be accepted'. That's explicitly the opposite of what you've posted. No one is going to bother to submit anything. Anyway, I think I've got what I needed here. Thanks. |
@petergoldstein I think it's unfair what you're doing here. Please never do it again 🙏 The project clearly communicates:
And not
Please understand that not every open-source project is the same! With all due respect, you claim to have no judgement of the work that's been put into this project, but here we are:
Consider taking a look at the history and conversations related to all those tickets linked in this PR. There's a lot of energy and support shared in those to motivate to not continue the legacy path and this is just one of the many ways this was approached: not accept changes in the old version to focus on the new stuff. Thanks |
@stas @petergoldstein I can relate to both of you guys and from what I see, it's just a bit of a misunderstanding that's reasonably easy to fix – so I've created a PR to address it. ;-) |
@stas Is there any tentative timeline for releasing this version? When should we expect it to get released? |
@nikhilgoyal22 to be honest, not really, as I feel like everyone is just happy with v2 🙈 My main concern re the current status is the public API and how the serializer class resolver works. I was going back and forth in my head re these two, and will probably refactor the Honestly, last thing I'd like to release is a version that will cause a bunch of issues for the Finally, there are parts of the documentation that are not finished, and I'd like to have it in place before we move on with a release. Please let me know if I should just go ahead and release things the way they are and take care of the rest in v3.1? 😃 |
I am not against a V3 rewrite; my only primary concern is if we were to lose the fine control that the V2 offers. For example, we may have 5+ different serializers for the same class for different user types or endpoints. I apologize if it came off as me saying not to innovate on this library. I am very much ok with added features, as long as they do not result in loss of control to the developer on what exactly is serialized and how. Most people are probably concerned about the rewrite because they can't easily change the application using this library if the API changes substantially. Just switching from fast_jsonapi took a month of work and another couple of weeks of testing for where I worked. I think you have done fantastic work, both for this Gem and the Ruby community. |
Hi, if you'd like people to test this out and give feedback, I'd suggest some things that would make this more likely (although certainly no guarantees) are:
Still there is of course no guarantee, testing out a 3.0.0 branch may just not be a priority for many users. I know developing open source is hard, I appreciate that this exists at all. But I'm not sure it's clear to anyone else what it is you are asking for in terms of contribution or help ro support. @petergoldstein's plan at #141 (comment) make sense to me... as it is, I understand you are feeling unsupported with regard to this branch/3.0, but I'm not totally sure what it is you are asking for, specifically. |
@stas I don't necessarily want to relitigate my comments from #141 (comment) , but I do want to say that I agree with @jrochkind 's comment above in #141 (comment) . Even after another 10+ months it's really unclear to me what the plan is for a 3.0 and what sort of assistance would be helpful in moving that along. I can personally live with the current state for my purposes (critical patches accepted against v2 and a license that allows a fork), but it doesn't seem like a great state for the project. If there's something specific that would be helpful, I'd ask that you please let the community know. |
@petergoldstein (or anybody who's down to help test this branch):
I'm really not feeling comfortable releasing such a major breaking change in the DSL/API without some more feedback from the community. Thank you 🙏 |
@stas Ok. That's helpful. I'll see what I can do early in the new year. I have a project or two where I should be able to fork and build/test with this PR branch. A couple of items:
|
Tests should pass, the docs coverage is still pretty low though.
Yeah, well, the biggest change is around how the serializers are resolved, and that's pretty well documented. In a nutshell, we just track which serializer inherits from the mixin, this way we know exactly how to identify the serializer based on a name/key. The downside for this is that it requires all the serializers to be loaded/tracked, before these can be resolved successfully... The part with the docs tbh is the biggest chunk of the effort that's left. (Assuming no API/DSL changes are necessary). |
@stas I'm getting local build failures (at least Rubocop) on this branch. I'll put together a PR with proposed fixes to review. To do that, I need to know what's the minimum Ruby version you want to support with the 3.0 version. I'd suggest Ruby 2.7 given that 2.4, 2.5, and 2.6 have all been EOLed for quite some time (and 2.7 will be EOLed in 3 months). Details here. What do you think? |
Yup, definitely not interested in supporting anything that's at EOL. Thank you @petergoldstein |
Great, thanks. I'll try to have a PR with passing CI this week. |
@stas So it looks like we can just delete the following subdirectories (these are the source of most, but not all, of the Rubocop issues):
Is this correct? It looks like everything under It looks like we'll then need to delete |
@stas If you can give me your thoughts on my previous comment I should be able to address all the outstanding Rubocop issues. I think I can just delete these files, but I want to make sure I'm not missing anything. |
Hey guys, thank you for this fork. Any news on this version? Maybe I can offer some help? |
Hey everyone! We're considering adopting this at our company as we redevelop our API to confirm to JSON:API spec. IS this project still active? I doesn't look like there's much movement on this V3 re-write. We'd love to contribute if possible and keep this alive as a viable gem. |
Heya @Kallin I don't know how much interest is out there to push for this rewrite, but definitely happy to split the responsibilities. You can also email me stas at nerd dot ro Ty ty for reaching out ❤️ |
I did a quick survey of the JSON API serializer landscape. A few that I've found:
I've probably missed some, sorry for that. List isn't exhaustive. We're currently using the deprecated Fast JSON API, but will need to replace it eventually. For the Ruby community, there has been many JSON API related gems (the list above aren't the only ones). While it's great with experimentation, there is also a cost to fragmentation. I think it would be good if the community could rally around 1-2 gems, and share the work of keeping them performant and up to date. The way I see it, there are two potential paths. Future directionThere are two paths to take for a good JSON API library in Ruby:
Both approaches has some pros and cons. For the first route, it's probably this gem, Graphiti or ActiveModelSerializers that are the best options to continue with. For the second route, perhaps Next stepsHappy to hear other peoples thoughts around this. I also think it would be good if a few companies could commit money (via e.g. Github Sponsors) to the project they'll go with. My employer would be willing to commit some recurring sponsorship. It'll be much cheaper for companies that need this, if they sponsor a project and share the work, instead of doing more work in-house, for lack of good community gems. PingPinging some people that may be interested in chiming in with their thoughts. Feel free to ping others that you think may be interested (both on the consumer side, i.e. those that are using the gem and my be willing to sponsor work on them, and people with detailed knowledge of JSON API and serialization). @GMorris-professional @lessthanjacob @jmeridth @ritikesh ( @bf4 @revis0r @dgeb ( @tagliala @beauby ( @adamcrown ( |
I'm either the maintainer or the author of the following work (some of my clients or startups use my work, so I'm still directly invested in the state of the projects below):
Along the years, I've tried doing the same you are trying to do now: consolidate the JSONAPI work under one project/team (see #144). Well, I've failed and my opinion is to not try to wait for the community to help and just run with it if you feel like. @sandstrom most of the projects shared by you never got any traction nor are performant nor are maintained by now. Some of these projects are also taking the Rails approach where things are magical and the end-user dev needs to learn some DSL to operate the implementation. There's no silver bullet tbh, and if you're looking to find it, I don't think this is the place to do it 🙈 @sandstrom I need to mention that I've never asked for compensation for my work, and I feel like it's awkward to start this conversation from this perspective (esp. having no previous contributions to any of these projects, not trying to be mean, just pointing the optics from my side). Again, I'm happy to share the responsibilities, but I don't want to start a political campaign and I'm likely to ask everyone kindly to not do it here (in this PR at least). Everyone who wants to work together has my email and I'm happy to help however I can. Hugs! |
In terms of AMS, I'm still maintaining it, even after years of not using it. So, I don't see why even if no one is developing this library, it can't be maintained. Discourse it still even using an ancient AMS. If you want something which does everything JSON:API and does it well https://github.com/cerebris/jsonapi-resources is a good bet. Caveat-- the latest release isn't yet out so it's a little in between right now. There's definitely other serialization libraries out there.. probably most of them at this point are responses to things which didn't go right in AMS, lol |
Tests pass, WIP on the code qa and rubocop.
The milestone issues: https://github.com/jsonapi-serializer/jsonapi-serializer/milestone/1
What is the current behavior?
Basically the main goal of this effort is to improve the situation around the following concerns
record_type
,type
,cached
and especially magic guessing around which serializer is to be used on a relationship (relationships serializer inference #14, Deprecate therecord_type
option on relationships #34, Polymorphic option should be either deprecated or documented better #140)lazy_load_data
confusion and help streamline the API for tasks like Allow conditional relationships to see what's been included #21, Convertid_method_name
toforeign_key
#45, set_id does not seem to be called for relationships #53, embedded relationship data does not honor relationship'sobject_method_name
#64, Conditional lazy_load_data for relationships #123, Lazy Load Data does not respect inclusion of nested associations #129 (lazy_load_data
overwritesinclude
and does not render relationship data #23)include
option #41)What is the new behavior?
There are a couple of architectural updates. Mainly, there should be less instantiations, since the definitions for attributes, relationships and links are moved into simple hashes. The codebase should get lighter by a 1-2 hundreds of lines of code. The codebase should reach 100% documentation coverage.
Another API improvement, is that we'll introduce a simple way to run serializations, by running something like:
On the DSL side of things, a bunch of options which are confusing or do the same thing (like
record_type
,serializer
andpolymorphic
) will unite under one option and will be strict about what is allowed to be passed.Performance wise, we're matching the previous number of instructions per second...
Configuration
We'd have to add the serializers path to the list of autoload paths in the Rails app
config/application.rb
:Checklist
Please make sure the following requirements are complete:
features)