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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify Schema/ResourceObject and Request/ResourceObject to improve developer experience #3

Open
luispabon opened this issue Jul 11, 2017 · 4 comments

Comments

@luispabon
Copy link

luispabon commented Jul 11, 2017

Just implementing some stuff using this library 馃憤

One thing I've found rather confusing is having one ResourceObject entity for received resources and a different ResourceObject entity for when we need to create/update. Not only it was unexpected, it's cumbersome to use (class imports, converting one type into another) and very prone to error.

Also, the Request/ResourceObject entity is write-only, and the Schema/ResourceObject entity is read-only. This severely limits what can be done with the entities.

Use cases this separation gets in the way of include:

  • Reading a resource from the API, modifying, then sending back for updating
  • Unit tests on code that needs to evaluate ResourceObjects
  • Referencing ResourceObject on classes that use both - always need to alias imports

These two should really be the same entity, I can't quite see any reason they should be different things entirely since they're virtually the same thing, and indeed on JSONAPI they're the same thing.

@kocsismate
Copy link
Member

kocsismate commented Jul 19, 2017

Hey there,

Thank you for your feedback! You are absolutely right that it's not so good that these classes share the same name. But I have to think about how to best solve the problem because there are some constraints:

  • Schema\ResourceObject has links and meta members, while Request\ResourceObject doesn't
  • They even don't have the same relationships: relationships in request must only contain a data member, while relationships in responses can have much more information
  • I like to follow the Interface Segregation principle. Here is an example: when you process the response, you will never call the setAttributes() or the setRelationships() method, that's why Schema\ResourceObject mustn't have them. This way the API becomes much easier to use: requests are writeable, responses are read-only.

So I'm not convinced at all that these classes should be merged together... But let's think a little bit:

  • In order to solve the third problem, maybe they could just be renamed to something different.
  • In order to handle your first problem a Factory Method Request\ResourceObject::createFromResponse(Schema\ResourceObject $resourceObject) could be added.
  • Another solution might be (which should solve all problems) to merge the two classes and create a RequestResourceObjectInterface and a ResponseResourceObjectInterface. I like it for the first sight, but I don't know if it really works.

I am curious about your opinion about what I've just written.

@luispabon
Copy link
Author

No, thank you for your hard work!

Another solution might be (which should solve all problems) to merge the two classes and create a RequestResourceObjectInterface and a ResponseResourceObjectInterface. I like it for the first sight, but I don't know if it really works.

I think that would work nicely. We've currently switched from our in-house JSONAPI client to yours and we used an approach similar to this. It helps streamlining developer experience when you don't need to be constantly converting back and forth.

@kocsismate
Copy link
Member

Sorry for being so late! I'll try to come up with a solution, but the Summer is a bit hectic for me :P 馃槑

@kocsismate
Copy link
Member

I started to experiment here with the last solution I proposed: #6

It's not ready yet, because I still have to work on the relationships (they also need two interfaces).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants