Skip to content

Conversation

@danielfdsilva
Copy link
Member

This PR implements my suggestion of centralizing the requests as explained in the comment #27 (comment)

@danielfdsilva danielfdsilva requested a review from AliceR December 2, 2025 18:03
Copy link
Member

@AliceR AliceR left a 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:

  1. Separation of concerns - StacApi describes where to make requests, hooks decide how to handle responses
  2. Flexibility - If a hook needs custom response handling (checking headers, status codes, ETags), it can access the raw Response
  3. Testability - Error handling logic is visible in tests without needing to mock the entire StacApi class
  4. 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?: StacApi instead of any)
  • 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?

@danielfdsilva
Copy link
Member Author

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:

static async handleResponse<T>(response: Response): Promise<T> {
// Some STAC APIs return errors as JSON others as string.
// Clone the response so we can read the body as text if json fails.
const clone = response.clone();
if (!response.ok) {
let detail;
try {
detail = await response.json();
} catch {
detail = await clone.text();
}
throw new ApiError(response.statusText, response.status, detail, response.url);
}
try {
return await response.json();
} catch (_) {
throw new ApiError(
'Invalid JSON Response',
response.status,
await clone.text(),
response.url
);
}
}

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:

queryFn: async () => {
const response = await fetch(url, {
headers: {
'Content-Type': 'application/json',
...options?.headers,
},
});
const stacData = await StacApi.handleResponse<{ links?: Link[] }>(response);
const doesPost = stacData.links?.find(
({ rel, method }: Link) => rel === 'search' && method === 'POST'
);
return new StacApi(response.url, doesPost ? SearchMode.POST : SearchMode.GET, options);

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.

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

Successfully merging this pull request may close these issues.

3 participants