Vertical Slice POC Feedback

brianperry → credited lukas.fischer → .

Migrating some great Slack feedback from @lukas.fischer to this issue...

I looked briefly into it: Obviously the POC is as POC. I think the ground structure is well done. We built it in a similar way actually. I miss a couple of things:
* Routing is besides loading collections super crucial. I’d work on that asap because then it’s more “real use case” than, abstract POC.
* As I understand, you currently don’t support isonapi includes - maybe I missed it. I think this is very crucial as well in a real use case. We decided to use drupal-jsonapi-params. Of course I can write my own params but I don’t want to - as this is the reason why I use a client lib.
* I think it’s important to build the client lib frontend framework agnostic. As I see you already allow custom fetch objects which is good.
* I’d recommend to build something like a playground which makes exploration of the current Drupal site possible. We are building something like this https://github.com/NETNODEAG/nodehive-js/tree/main/playground so developers can test and explore the client library.
* Why did you add a cache? I would not do that, as it’s up to the implementation if you want that. We usually use nextjs with app router, and there we would not want to have an additional cache layer in the client lib. Wondering why you added that.

Full thread here: https://drupal.slack.com/archives/C05BP6659U0/p1700139339779949

Responding to @lukas.fischer's feedback

> I think the ground structure is well done. We built it in a similar way actually.

Very helpful to hear that. Thanks!

> Routing is besides loading collections super crucial. I’d work on that asap because then it’s more “real use case” than, abstract POC.

When you say routing here, are you referring to the equivalent of your client's router method: `client.router('/node/104')`? I agree that is an important use case, but it is also something that isn't well handled by core at the moment. I'm assuming that your implementation depends on the Decoupled Router module. I'm expecting this project to provide utilities to work with decoupled router, we just need to figure out how we want to handle things that are not part of core. We have an existing issue for router support here: https://www.drupal.org/project/api_client/issues/3398403 →

> As I understand, you currently don’t support isonapi includes - maybe I missed it. I think this is very crucial as well in a real use case. We decided to use drupal-jsonapi-params. Of course I can write my own params but I don’t want to - as this is the reason why I use a client lib.

The POC does support includes, but this feedback is helpful in that we probably need to provide better documentation around this and also how developers can use Drupal JSON:API Params with this library. Our current api accepts a `queryString` option which is intended to to be the full query string passed to JSON:API. Drupal JSON:API Params has a method to output a query string, so you can use that package to assemble your query, and then output the result to the client. A little more detail here: https://project.pages.drupalcode.org/api_client/interfaces/_drupal_api_c...

> * I think it’s important to build the client lib frontend framework agnostic. As I see you already allow custom fetch objects which is good.

We agree and are making sure this library is framework agnostic. We expect others to extend this in order to layer on framework specific functionality.

> I’d recommend to build something like a playground which makes exploration of the current Drupal site possible. We are building something like this https://github.com/NETNODEAG/nodehive-js/tree/main/playground so developers can test and explore the client library.

We are planning on doing this as well, but are currently prioritizing the implementation. Thanks for sharing the example from your client. We also have a related issue here: https://www.drupal.org/project/api_client/issues/3379156 →

> * Why did you add a cache? I would not do that, as it’s up to the implementation if you want that. We usually use nextjs with app router, and there we would not want to have an additional cache layer in the client lib. Wondering why you added that.

Our plan is to keep the cache optional and not enabled by default. So this should not force you to use the cache for this client or prevent you from using Next's app router caching. Our assumption is that supporting caching could be useful in some use cases and it was also a feature of many of the clients that we are considering when designing this client. If the community consensus is that this feature isn't useful, it would reduce complexity, so we can continue to keep an eye on the need here.

Thanks again for this feedback. Extremely valuable.

Closing as we've reached 1.0.

Automatically closed - issue fixed for 2 weeks with no activity.