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
Proposal: Simplify response model to avoid Converting circular structure to JSON
error
#5332
Comments
I seem to have fixed this in v1.2.0 (#5247). Is this issue still relevant for the latest version? |
We're certainly seeing circular reference issues in production, and it's not clear what is causing the problem, only that it happens intermittently, and the fact a
For reference, to match line numbers up, here's the 1.3.3 tag: https://github.com/axios/axios/blob/v1.3.3/dist/node/axios.cjs What I believe is happening, is that in The defect here is that the Could |
Same problem here.
|
@spaskhalov Did you manage to solve it? I hit the same problem. |
@nofearOnline same here, if anyone has any pointers, happy to hear them. I only happens on an error. |
@nofearOnline the problem only occurs if the exception is not handled. So I just rewrote my tests so that in cases where I want to check that my methods throw exceptions, I wrap them in a try catch and check that all checks were passed with |
I'm also getting the occasional report of a similar error: TypeError: Converting circular structure to JSON I'm using an https.agent to include a local certificate. Strange that it only occurs a few times out of many successful calls. I try to catch the error with a try/catch but it is still crashing my app. |
Axios error handling documentation references using It's not clear if For the sake of completeness, other recommendations I came across:
|
Summary: [BizSDK][NodeJS] Remove dependency request from package.json `request` package has long been deprecated and shall be replaced. Remove `request` and `request-promise` dependencies. Use `axios` as a replacement. Update `api.js` and `http.js` during migration. Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332. Reviewed By: mengxuanzhangz Differential Revision: D48271770 fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
Summary: [BizSDK][NodeJS] Remove dependency request from package.json `request` package has long been deprecated and shall be replaced. Remove `request` and `request-promise` dependencies. Use `axios` as a replacement. Update `api.js` and `http.js` during migration. Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332. Reviewed By: mengxuanzhangz Differential Revision: D48271770 fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
Just faced this issue, this proposal helped me find the issue. On a project with NestJS, I started returning an axios result, instead of result.data |
Summary: [BizSDK][NodeJS] Remove dependency request from package.json `request` package has long been deprecated and shall be replaced. Remove `request` and `request-promise` dependencies. Use `axios` as a replacement. Update `api.js` and `http.js` during migration. Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332. Reviewed By: mengxuanzhangz Differential Revision: D48271770 fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
Is your feature request related to a problem? Please describe.
It is common practice to log every request and response from an application, using JSON.stringify. However, this doesn't work in Axios because the response model is very complex (it contains a copy of the request and cyclical types). I am not the first person to experience this based on the number of issues and StackOverflow questions raised about this:
It is quite time consuming to research this issue to identify the solution - that only
response.data
is convertable to JSON so the code should be changed toJSON.stringify(response.data)
. In addition, this is only a subset of the information needed to be logged (e.g. HTTP response code, etc), so for developers to correctly log the Axios response, it is something like:JSON.stringify({ data: response.data, status: response.status, statusText: response.statusText, headers: response.headers )
or alternative we could delete the fields within response that don't belong there, namely
config
andrequest
, but then that means mutating the response, i.e.Describe the solution you'd like
The AxiosResponse should contain only those fields that are part of the response, so I propose that
config
andrequest
are removed. It is not the responsibility of theAxiosResponse
object to contain the corresponding request - if needed, a new object such asAxiosOperation
can be created to contain the request, response and config.In addition, the config and request fields should be examined for circular dependencies and the modelling adjusted to avoid them, since it causes unnecessary confusion to have a circular structure.
Obviously this is a massive breaking change, but given the issues that this causes in many downstream libraries and services that use Axios, the severity of this issue should not be understated.
Describe alternatives you've considered
We could consider creating a custom function to JSONify the response or could try to mitigate this issue via documentation, but I don't believe that the adoption of either option would be easy given the huge number of projects and libraries that use Axios.
Additional context/Screenshots
No response
The text was updated successfully, but these errors were encountered: