fix: fixed BigInt sent as json

NikoRaisanen commented 1 year ago

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: https://github.com/ladjs/superagent/issues/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.

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!

titanism commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

bjornua commented 1 year ago

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

Sure, I'll have a look

ladjs / superagent