Skip to content

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Rewrite (v3.0.0) #141

wants to merge 1 commit into from

Conversation

stas
Copy link
Collaborator

@stas stas commented Oct 23, 2020

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

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:

JSONAPI::Serializer.serialize([user, actor, movie]).as_json

On the DSL side of things, a bunch of options which are confusing or do the same thing (like record_type, serializer and polymorphic) 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:

config.autoload_paths << 'app/serializers'

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

@stas stas force-pushed the rewrite branch 3 times, most recently from 42f8de9 to 6e92450 Compare October 25, 2020 20:52
@stas stas linked an issue Oct 29, 2020 that may be closed by this pull request
@arinhouck
Copy link

arinhouck commented Nov 5, 2020

@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 include input to the tree structure or relationship key of the serializer relationship (for multi-level and polymorphic or STI relationships that are defined more than once)

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)

    has_many :memberships, if: lambda { |record, params, tree_structure|
      # defined on TeamSerializer
      # assuming we pass include option on serializer to params[:include] (kind of odd API to me but seems like only option from current code)
      # tree_structure = could be "relationship_a.team.memberships" or "relationship_b.team.memberships"
      params[:include].include?(tree_structure)
    }

So if I passed include: ["relationship_a.team", "relationship_a.team.memberships"]. I would only expect those following relationships defined via options[:include] to populate relationships hash and put records into include array.

I hope that makes sense. Happy to explain more.

@stas
Copy link
Collaborator Author

stas commented Nov 6, 2020

@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)
Copy link

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.

Suggested change
mapping[object.class] || NotFoundError.new(object, mapping.values)
mapping[object.class] || raise NotFoundError.new(object, mapping.values)

@petergoldstein
Copy link
Contributor

@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:

  1. Update the message on the README to state that you are not accepting new features in v2, but bugfixes, etc. will be accepted
  2. See if there are any bugfixes or maintenance needs (like getting new Rubies into CI) that are applicable to v2, and get those into v2. Publish a patch version on v2.
  3. Rebase this branch against master and get it mergeable
  4. Fix the tests in this branch, so it shows as green.
  5. Build a checklist for items needed to get to release of a v3 and publish in this PR, so it's clear what's the outstanding work. I know you say it's ~90% done, but that's not clear at all from what's here.
  6. Tick down that list, keeping the PR green, at whatever pace makes sense given your time and the time of any contributors who are interested.
  7. Publish v3 when ready.

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.

@stas
Copy link
Collaborator Author

stas commented Feb 2, 2022

@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.

@petergoldstein
Copy link
Contributor

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.

@stas
Copy link
Collaborator Author

stas commented Feb 2, 2022

@petergoldstein I think it's unfair what you're doing here. Please never do it again 🙏

The project clearly communicates:

At the moment, contributions are welcome only for v3!

And not

Contributions not welcome!

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:

  • you criticizing the new version of the project was conducted and telling everyone how you'd like it to be maintained
  • you not being interested in v3, but hijacking the PR conversation with all of the above

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

@clemens
Copy link
Contributor

clemens commented Apr 4, 2022

@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. ;-)

#214

@nikhilgoyal22
Copy link

@stas Is there any tentative timeline for releasing this version? When should we expect it to get released?

@stas
Copy link
Collaborator Author

stas commented Apr 14, 2022

@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 jsonapi/serializer/trackable.rb

Honestly, last thing I'd like to release is a version that will cause a bunch of issues for the v2 users. And historically, this is the most error-prone layer: #14 #34 #140

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? 😃

@ChrisHampel
Copy link

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.

@jrochkind
Copy link

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:

  • Clear docs on what's new, and especially what breaking changes might be included; ideally the new 3.0.0 README too.
  • A rubygems release of say 3.0.0.alpha1 might send a signal.
    • But I guess tests in this branch aren't currently passing, so it's not a release yet.

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.

@petergoldstein
Copy link
Contributor

@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.

@stas
Copy link
Collaborator Author

stas commented Dec 27, 2022

@petergoldstein (or anybody who's down to help test this branch):

  • run the gem locally, mainly play with the API/interface changes
  • review the lib/jsonapi/serializer/trackable.rb, I have a feeling that's not going to work well in most of the environments
  • review integration tests, it should reflect pretty well the changes in how the serializers are configured

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 🙏

@petergoldstein
Copy link
Contributor

@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:

  1. Does CI currently pass on this branch? If not, I'd probably want to get it to passing before engaging in manual testing. I'd be happy to help with that.
  2. Would you be able to draw up a draft 3.0 README as @jrochkind describes and include it on this PR? It would be really helpful to understand not only the code, but the intentions and thinking behind the changes.

@stas
Copy link
Collaborator Author

stas commented Dec 28, 2022

Does CI currently pass on this branch? If not, I'd probably want to get it to passing before engaging in manual testing. I'd be happy to help with that.

Tests should pass, the docs coverage is still pretty low though.

Would you be able to draw up a draft 3.0 README as @jrochkind describes and include it on this PR? It would be really helpful to understand not only the code, but the intentions and thinking behind the changes.

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).

@petergoldstein
Copy link
Contributor

@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?

@stas
Copy link
Collaborator Author

stas commented Dec 28, 2022

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

@petergoldstein
Copy link
Contributor

Great, thanks. I'll try to have a PR with passing CI this week.

@petergoldstein
Copy link
Contributor

@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):

  • lib/fast_jsonapi
  • lib/extensions

Is this correct? It looks like everything under lib/fast_jsonapi has been replaced with equivalents under lib/json/serializer except for the Railtie.

It looks like we'll then need to delete lib/fast_jsonapi.rb. Is that also correct?

@petergoldstein
Copy link
Contributor

@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.

@ghost
Copy link

ghost commented Jun 15, 2023

Hey guys, thank you for this fork. Any news on this version? Maybe I can offer some help?

@Kallin
Copy link

Kallin commented Nov 21, 2023

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.

@stas
Copy link
Collaborator Author

stas commented Nov 21, 2023

Heya @Kallin
happy to help and share the maintenance burden.

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 ❤️

@sandstrom
Copy link

sandstrom commented Nov 22, 2023

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 direction

There are two paths to take for a good JSON API library in Ruby:

  1. Consolidate effort around a dedicated JSON API gem (should it handle de-serialization and controller concerns too?)
  2. Work with one of the more general gems, on adding the building blocks and primitives necessary to support common JSON API use-cases.

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 blueprinter (or maybe cache-crispies, but probably harder).

Next steps

Happy 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.

Ping

Pinging 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 (blueprinter)

@bf4 @revis0r @dgeb (active_model_serializers)

@tagliala @beauby (jsonapi-rb)

@richmolj @wagenet (graphiti)

@adamcrown (cache-crispies)

@stas
Copy link
Collaborator Author

stas commented Nov 22, 2023

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!

@bf4
Copy link
Contributor

bf4 commented Nov 23, 2023

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
People haven't contributed to the AMS 'what else is there' in a while but I see it mentions https://github.com/okuramasafumi/alba and I think https://github.com/yosiat/panko_serializer is still probably a thing as are https://github.com/ismasan/oat and https://github.com/trailblazer/roar-jsonapi and then there's grape, rabl, and jbuilder...

@jsonapi-serializer jsonapi-serializer locked as off-topic and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge jsonapi-serializer with jsonapi.rb ?