-
Notifications
You must be signed in to change notification settings - Fork 4
Centralize stac api requests #30
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
base: issue#25/fetch-caching
Are you sure you want to change the base?
Conversation
AliceR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the implementation @danielfdsilva! I appreciate the effort, but after thinking through the implications, I'd like to keep the current architecture (as in #27).
The core principle: StacApi should be a thin routing layer to STAC endpoints, not a smart client with response handling logic.
Why this matters:
- Separation of concerns -
StacApidescribes where to make requests, hooks decide how to handle responses - Flexibility - If a hook needs custom response handling (checking headers, status codes, ETags), it can access the raw
Response - Testability - Error handling logic is visible in tests without needing to mock the entire
StacApiclass - Clarity - When reading a hook, you see exactly what happens: request → response → parse → error handling
What I do like from your PR is
- The TypeScript typing improvements in
context.ts(stacApi?: StacApiinstead ofany) - The test improvements using
new Response() - Better typing discipline in general
Eventually, if we add more hooks and the ~10 lines of error handling feels repetitive, we could create a utility function:
// src/utils/handleStacResponse.ts
export async function handleStacResponse<T>(response: Response): Promise<T> {
if (!response.ok) {
let detail;
try {
detail = await response.json();
} catch {
detail = await response.text();
}
throw new ApiError(response.statusText, response.status, detail, response.url);
}
try {
return await response.json();
} catch (error) {
throw new ApiError(
'Invalid JSON Response',
response.status,
`Response is not valid JSON: ${error instanceof Error ? error.message : String(error)}`,
response.url
);
}
}Then hooks could use it optionally:
const response = await stacApi.getCollection(collectionId);
return handleStacResponse<Collection>(response);This gives us reusable logic without changing StacApi's contract. Hooks that need custom handling can still work with the raw Response.
Regarding your comment on "setting patterns":
I understand your concern about establishing patterns for future development. However, I think the current pattern is actually the right one to follow! It demonstrates that hooks own their data transformation logic, not the API client.
If we're concerned about developers misunderstanding why we repeat this code, we could add a comment in StacApi:
/**
* StacApi provides endpoint routing only.
* Response handling and parsing is intentionally left to the calling hooks
* to allow flexibility in error handling and response processing.
*/
class StacApi {
// ...
}This makes the architectural decision explicit rather than hiding it behind abstraction.
Would you be open to extracting just the typing improvements into a smaller PR?
|
I appreciate the thorough response, but I do not think we have the right approach. I don't think stacApi is just a thin routing layer given that is does a lot of handling like with the search request for example. If this class is just a holder of utility functions that we should review it's existence altogether. StacApi should simplify the access to a STAC API for the user, this also means that error handling should be consistent. This is what is being done with this approach. The api makes the requests and ensures errors are handled consistently. The utility function you suggest is currently in the class: stac-react/src/stac-api/index.ts Lines 96 to 121 in ccc76da
Having to access the raw response should not be something necessary (also for a final user is not even possible since it's hidden by the hooks, so this is just for developers) - having a library means shielding the user from these details. But it is possible as it is being done in the `useStacApi´ hook: stac-react/src/hooks/useStacApi.ts Lines 16 to 29 in ccc76da
I believe there should be little reason why a hook would need a different response handling. Building a library also means abstracting implementation details away from the user, which we won't do if each hook has to implement its own handling. |
This PR implements my suggestion of centralizing the requests as explained in the comment #27 (comment)