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

Remove AMS and replace with simple json rendering #681

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

chvp
Copy link
Member

@chvp chvp commented Nov 9, 2024

AMS is unmaintained, and gives errors with Rails 8. The main reason to switched to it in the first place was performance, but the main issue with the performance was the use of partials. This performance issue can also be avoided by moving common functionality to a helper instead of a partial.

@chvp chvp added the chore Repository or build maintenance label Nov 9, 2024
@chvp chvp requested a review from robbevp November 9, 2024 14:26
@chvp chvp force-pushed the chore/remove-ams branch 3 times, most recently from e44b95e to 5c2fcb9 Compare November 9, 2024 15:04
@chvp chvp mentioned this pull request Nov 9, 2024
Copy link
Member

@robbevp robbevp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan of how this approach creates two files for every object that we serialize. Two suggestions that we could maybe try out:

  1. Did you look into other gems that provide serialization? I've also use blueprinter in projects, and liked their approach.
  2. Can we just skip jbuilder and place the tiny bit of logic in the controller? Something like render json: @albums.map { |a| transform...(album) }
    If that works, we might also be able to place those transform function as a private method in the controller

It might also be worthwhile do benchmark the performamce of these approaches, since that could also influence our decision

@chvp chvp changed the title Replace AMS with jbuilder Remove AMS and replace with simple json rendering Nov 11, 2024
@chvp
Copy link
Member Author

chvp commented Nov 11, 2024

You're right that we can just get rid of jbuilder as well. We don't have any complex use cases here, so I wouldn't introduce another dependency. Didn't do any performance benchmarking, but I can't imagine that this approach can be slower than doing this through a dependency.

@chvp chvp requested a review from robbevp November 11, 2024 14:33
@robbevp robbevp merged commit 6906768 into main Nov 11, 2024
5 checks passed
@robbevp robbevp deleted the chore/remove-ams branch November 11, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository or build maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants