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

Consider migrating model serializer of API from JBuilder to AMS (Active model serializer) #2973

Open
nvh0412 opened this Issue Nov 23, 2018 · 10 comments

Comments

Projects
None yet
7 participants
@nvh0412
Contributor

nvh0412 commented Nov 23, 2018

We're using JBuilder too long, and its performance is the one that we can improve by using AMS. Spree didn't migrate JBuilder to AMS, just because AMS is not ready at the time they discuss it.

AMS v0.10 is stable now, and I used it in many production environments, so my suggestion is migrating to ASM for all our API endpoint to improve our response time. At least we can save ~100ms for each one, I believe.

Really glad to receive any feedbacks about this issue.

@tvdeyen

This comment has been minimized.

Member

tvdeyen commented Nov 23, 2018

Panko is a AMS compatible serializer that’s even faster than AMS.
https://yosiat.github.io/panko_serializer/performance.html

@nvh0412

This comment has been minimized.

Contributor

nvh0412 commented Nov 24, 2018

Good one @tvdeyen, have you ever use Panko for the production environment? just want to know does it have any limitation.

@yosiat

This comment has been minimized.

yosiat commented Nov 25, 2018

hi,
I am the creator panko and we are using at production environment,
If can help, feel free to ask any questions

@jarednorman

This comment has been minimized.

Member

jarednorman commented Nov 26, 2018

One issue that we encountered in previous discussions about AMS is that it used global configuration, meaning that apps would be stuck using it the way we configured it, or their configuration would override and potential break the admin and stuff.

I personally work on stores that are using AMS (though honestly haven't been super happy with it) where this would be an issue.

For some personal opinion/anecdotal info, most stores I've worked with built out custom APIs for any storefront related logic they needed, so the performance of the Solidus API has never been a problem for me. I prefer using the Rails-endorsed solution for building JSON, jbuilder, for maintenance reasons.

@tvdeyen

This comment has been minimized.

Member

tvdeyen commented Nov 26, 2018

Supergood points @jarednorman. I did not thought of that tbh. Yes, extentability is an issue with AMS and much easier with jbuilder (as longs as you don't need to override the controller action anyway)

@nvh0412

This comment has been minimized.

Contributor

nvh0412 commented Nov 26, 2018

thanks @tvdeyen @jarednorman for this discussion, closing issue.

@nvh0412 nvh0412 closed this Nov 26, 2018

@BenMorganIO

This comment has been minimized.

Contributor

BenMorganIO commented Nov 26, 2018

I'm 👍 on moving away from JBuilder.

@jacobherrington

This comment has been minimized.

Member

jacobherrington commented Nov 27, 2018

Let's keep this issue open for discussion.

@BenMorganIO

This comment has been minimized.

Contributor

BenMorganIO commented Nov 27, 2018

TBH if we change up the API a bit more, we could just use to_json/as_json and remove the need for a JSON serialization dependency. It would be hard work, but probably worth it on a few endpoints. Like, the main problem we keep having is: we want to to use a thing that makes the API look nice. We could try getting rid of that altogether.

@ericsaupe

This comment has been minimized.

Contributor

ericsaupe commented Dec 5, 2018

Netflix also has a serializer https://github.com/Netflix/fast_jsonapi. Adding that link just to give another option.

We ran into a few areas where we wanted more data from the API that wasn't given. I'd like to see some improvements made to the API and serializing models might be useful.

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