openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.09k stars 708 forks source link

PoC migration to fast_jsonapi #5654

Open luisramos0 opened 4 years ago

luisramos0 commented 4 years ago

What we should change and why (this is tech debt)

AMS is not mantained anylonger we need to move to something else. Discussion here: https://community.openfoodnetwork.org/t/alternatives-to-ams/1717/

We need to try fast_jsonapi and see how it would look like. From what I read it will consist of minor adaptions in the serializers and some work to adapt the frontend code to the new jsonapi compatible payloads :tada:

Ideally, this issue includes more info, or even a second POC, using other alternatives jbuilder or blueprinter or other.

Context

Last raised in https://github.com/openfoodfoundation/openfoodnetwork/pull/5638

Impact and timeline

AMS works with rails 6, but if we know we will need to change why not adapt now.

jeduardo824 commented 3 years ago

Hey, I can try to move some serializers (I don't know how many) to fast_jsonapi as a POC and check if will be simple to move from AMS, what do you guys think?

luisramos0 commented 3 years ago

yes please, that would be amazing!

jeduardo824 commented 3 years ago

Ops, I think we will need to wait a little bit more before doing that because fast_jsonapi (now renamed to jsonapi_serializer) needs Rails 4.2, and we are at 4.1.

luisramos0 commented 3 years ago

Hello @jeduardo824 I guess you mean https://github.com/jsonapi-serializer/jsonapi-serializer/
I think you are right, even fast_jsonapi started with rails 5 and then moved to support rails 4.2 :face_with_head_bandage: This is blocked by rails 4.2 upgrade. For what is worth, I have now created the rails 4.2 upgrade issue :-) https://github.com/openfoodfoundation/openfoodnetwork/issues/5761

luisramos0 commented 3 years ago

Anyway, thanks a lot, it's really good we have a specific plan in this area :+1:

jeduardo824 commented 3 years ago

Awesome! I will be happy to help with the 4.1 and 4.2 migration if you guys need anything else (I saw that almost all tickets are assigned, so I don't know if there is anything else to do with the 4.1 migration) and after that, I can move forward with the POC of jsonapi-serializer

luisramos0 commented 3 years ago

:ok_hand:

If you look at the latest/best 4.1 build on PR #5611 https://semaphoreci.com/openfoodfoundation/openfoodnetwork-2/branches/pull-request-5611/builds/4 You can see other issues to be solved. Including instances of #5606 with errors like: ActionController::InvalidCrossOriginRequest

Matt-Yorkley commented 3 years ago

Note: Netflix are not the primary maintainers any more, the project has been forked https://github.com/jsonapi-serializer/jsonapi-serializer

luisramos0 commented 3 years ago

This is not blocked any more. It is worring that v3 will be a "rewrite"... https://github.com/jsonapi-serializer/jsonapi-serializer/pull/141

Matt-Yorkley commented 3 years ago

I had a quick look at what implementing this would actually involve, and I think it might be a lot more work than we imagined. The output is very closely tied to the JSON:API standard, and I think it would necessitate rewriting a lot of our existing Angular code, specifically in that the attributes of the objects are additionally nested under an attributes: key: https://github.com/jsonapi-serializer/jsonapi-serializer#serialized-output

The 20-30x improvement in performance is pretty crazy though...

kirstenalarsen commented 3 years ago

There's no way to do it one-by-one @Matt-Yorkley @luisramos0 ? Like rewrite the angular code in relation to each endpoint as we release them? or it would all have to be done to change to the new thing before we can do anything?

Matt-Yorkley commented 3 years ago

The issue is: exactly how much rewriting would be needed? We won't know that without doing a bit of a spike.