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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetchEntries does not throw error #3696

Open
jbean96 opened this issue Oct 29, 2024 · 2 comments
Open

fetchEntries does not throw error #3696

jbean96 opened this issue Oct 29, 2024 · 2 comments

Comments

@jbean96
Copy link

jbean96 commented Oct 29, 2024

Describe the bug

The SDK function fetchEntries does not throw an error which makes it challenging to handle the occasional API call failure on the consuming side.

Specifically in our case we use @tanstack/query to handle our data loading/caching in our applications, since the function always is resolve-ing the Promise, we aren't able to determine if the function succeeded or failed and thus the query that is using this function indicates that it was successful. Downstream this causes us potential NPEs because we're expecting an Object/Array when the query is in a successful state, not null as is returned from this function.

To Reproduce

Not including because it's not necessary, you can look at the function definition and see this is the case. I've included it below for reference.

/**
 * Returns a paginated array of entries that match the given options.
 */
export async function fetchEntries(options: GetContentOptions) {
  try {
    const url = generateContentUrl(options);
    const content = await _fetchContent(options);

    if (!checkContentHasResults(content)) {
      logger.error('Error fetching data. ', { url, content, options });
      return null;
    }

    return _processContentResult(options, content);
  } catch (error) {
    logger.error('Error fetching data. ', error);
    return null;
  }
}

Expected behavior

The function should instead throw an Error in the error handling cases instead of returning null.

Screenshots

N/A

Additional context

N/A

@samijaber
Copy link
Contributor

You're right, we shouldn't be swallowing errors in this way.

We'll work on throwing errors. This will need to be a major version bump due to breaking changes.

@jbean96
Copy link
Author

jbean96 commented Oct 30, 2024

You're right, we shouldn't be swallowing errors in this way.

We'll work on throwing errors. This will need to be a major version bump due to breaking changes.

Totally understood on the major version bump, looking forward to seeing the update! We just wrote a wrapper function for now that throws an error if the response is null... which obviously has flaws but works OK in our case.

Looking forward to updating once y'all release on your end though :) Thanks!

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