-
Notifications
You must be signed in to change notification settings - Fork 52
Fix circular JSON marshalling errors #131
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: master
Are you sure you want to change the base?
Conversation
|
@Skitionek does this address the three issues and deal with the error in the failure? |
Skitionek
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.
Thank you for contribution.
It looks good to me, I will try to find the time over weekend to merge and release this fix.
src/MSTeams.js
Outdated
| // Create a safe representation of the response to avoid circular reference errors | ||
| const safeResponse = {}; | ||
|
|
||
| // Safely copy properties, handling potential circular references | ||
| try { | ||
| safeResponse.status = response?.status; | ||
| safeResponse.statusText = response?.statusText; | ||
| safeResponse.headers = response?.headers ? JSON.parse(JSON.stringify(response.headers)) : undefined; | ||
| safeResponse.data = response?.data ? JSON.parse(JSON.stringify(response.data)) : undefined; | ||
| } catch (circularError) { | ||
| // If we still hit circular references, just include basic info | ||
| safeResponse.status = response?.status; | ||
| safeResponse.statusText = response?.statusText; | ||
| safeResponse.error = 'Response contained circular references'; | ||
| } | ||
|
|
||
| throw new Error('Failed to send notification to Microsoft Teams.\n' + 'Response:\n' + JSON.stringify(safeResponse, null, 2)); |
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.
This is fair solution 👍
|
I've added this to an action to test it, and there are still issues. The response is now being printed, and showing a status 200. That would suggest that the issue is elsewhere, likely due to this previous commit 39a7b67 |

Fixes #118 and fixes #125 and fixes #126.
Adds a test case to ensure that this is correct.