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

fix: fixed BigInt sent as json #1773

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

NikoRaisanen
Copy link
Contributor

@NikoRaisanen NikoRaisanen commented Jul 26, 2023

Checklist

  • [ X ] I have ensured my pull request is not behind the main or master branch of the original repository.
  • [ X ] I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • [ X ] I have written a commit message that passes commitlint linting.
  • [ X ] I have ensured that my code changes pass linting tests.
  • [ X ] I have ensured that my code changes pass unit tests.
  • [ X ] I have described my pull request and the reasons for code changes along with context if necessary.

Problem

Inspired by this issue: #1769
When a value containing primitive type BigInt is sent with type set to json, the error message is sent in the request body. I believe that this kind of error should be caught before the request is sent out, so that errors can be handled internally.

The error occurs when a BigInt or object containing a BigInt is sent with type set to json, 2 examples below:

superagent.post(url)
  .type('json')
  .send(BigInt("1"))
  .end(...)
superagent.post(url)
  .type('json')
  .send({ number: BigInt("1") })
  .end(...)
Previous behavior:

JSON error occurs, but the error message is sent in the body of the request regardless. I have a local mockoon server running on the left where you can see what was received by the server.
image

Behavior after this PR:

Error is thrown if BigInt is found in the .send() method, error not sent to server

Error: Cannot serialize BigInt value to json at RequestBase.send (F:\superagent-test\node_modules\superagent\lib\request-base.js:616:47) at testAsync (file:///F:/superagent-test/index.js:26:14) at file:///F:/superagent-test/index.js:43:7 at ModuleJob.run (node:internal/modules/esm/module_job:185:25) at async Promise.all (index 0) at async ESMLoader.import (node:internal/modules/esm/loader:281:24) at async loadESM (node:internal/process/esm_loader:88:5) at async handleMainPromise (node:internal/modules/run_main:65:12)

It's my first time with an open-source contribution so let me know what you would like changed and I'd be happy to do it!

@NikoRaisanen NikoRaisanen marked this pull request as ready for review July 26, 2023 21:19
@NikoRaisanen NikoRaisanen changed the title BUG FIX: Throw error for cases where BigInt used w/ type('json') fix: fixed BigInt sent as json Jul 26, 2023
@titanism titanism merged commit a62866a into ladjs:master Aug 15, 2023
@titanism
Copy link
Collaborator

v8.1.0 released which fixes this issue, thank you

npm install superagent@8.1.0

release notes @ https://github.com/ladjs/superagent/releases/tag/v8.1.0

@bjornua
Copy link
Contributor

bjornua commented Aug 15, 2023

This broke some code I'm working on, where I use

BigInt.prototype.toJSON = function (key, value) {
  return String(this);
};

to generally make BigInt serialize in many legacy libraries. I realize it's a hack, but it works pretty well except for when upgrading to superagent@8.1.0

@titanism
Copy link
Collaborator

Is there a way you could do a PR or something to fix this @bjornua?

@bjornua
Copy link
Contributor

bjornua commented Aug 15, 2023

Is there a way you could do a PR or something to fix this @bjornua?

Sure, I'll have a look

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