search

farmOS / farmOS.js

A JavaScript library for working with farmOS data structures and interacting with farmOS servers.
MIT License
15 stars 13 forks source link

Add `.getId()` method #40

Open jgaehring opened 3 years ago

jgaehring commented 3 years ago

Similar to farmOS/farmOS.py#42.

This is by no means a blocker, since it's not really necessary for anything on the FK Roadmap (farmOS/farmOS-client#373), and it can always be added at any time down the road w/o being a breaking change, but it's such low hanging fruit and a significant optimization it's worth considering.

jgaehring commented 2 years ago

I may wait on this a little longer. I guess I'm just reluctant to add another method like this, expanding the API surface.

As an alternative, I wonder if it might make better sense to add an id parameter to the fetch options, which takes a single UUID. I don't know if that is any better, honestly, but it at least feels more aligned with the current API design.

paul121 commented 2 years ago

As an alternative, I wonder if it might make better sense to add an id parameter to the fetch options, which takes a single UUID. I don't know if that is any better, honestly, but it at least feels more aligned with the current API design.

I would really recommend against this. The response type is different when you GET a single id as a path argument. From the original farmOS.py issue:

Another reason to move the ID to a separate method is because the response of a single resource is different than the response of multiple resources. Fetching a single resource, such as /api/log/observation/f1790e98-8762-4c16-bb93-ef53f0f77dfe returns a single object in the response data property. Fetching a collection of resources returns an array in the data property.

jgaehring commented 2 years ago

I would really recommend against this. The response type is different when you GET a single id as a path argument

Ohhhh, yes, thank you! I was struggling to remember our conversation around this, and yes, I definitely don't want to do this.

Ok, I think I will punt on this though for now, at least until there is a clear need for such a method. I'd be inclined to implement it in the standalone export of the client, but removing it via adapter, so it is not present on the main farmOS instance.