Skip to content

Conversation

@mocca102
Copy link
Contributor

Why?

  • Focus API events return a text response for the 429 errors
  • We only handle JSON response types
  • This PR adds support for handling text responses as well by checking the Content-Type header before processing the response

The screenshot shows an example of the API response for 429 response

Screenshot 2025-04-15 at 9 47 44 PM Screenshot 2025-04-15 at 9 47 28 PM

@mocca102 mocca102 requested a review from a team April 15, 2025 19:58
Copy link
Contributor

@Mudaafi Mudaafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one comment.

Note: the trackConversion test for custom_type is currently failing due to Backend changing the error response.

Comment on lines +8148 to +8154
fetchSpy = sinon.spy(() => Promise.resolve({
ok: false,
status: 429,
statusText: 'Too Many Requests',
headers: new Map([['content-type', 'text/plain']]),
text: () => Promise.resolve('Too many requests'),
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the mock here

Comment on lines 179 to 184
}).catch((error) => {
instance.eventemitter.emit('error', {
url,
method,
message: `Error reading text: ${error.message}`,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we standardize the the error handling for these two paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mudaafi Sorry what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I meant that we catch and emit the error for .json() and .text() very similarly. Possibly something we can extract into a handler function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You're right. Let me do that

@mocca102 mocca102 requested a review from Mudaafi April 16, 2025 20:48
Copy link
Contributor

@Mudaafi Mudaafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, thanks for doing the refactor. If @esezen has some time to look over the changes just so we didn't miss anything, that'd be great.

@Mudaafi Mudaafi requested a review from esezen April 16, 2025 22:05
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@esezen esezen merged commit f424edd into master Apr 17, 2025
6 of 7 checks passed
@esezen esezen deleted the ci-4305-node-sdk-handle-api-text-response branch April 17, 2025 11:37
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.

4 participants