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

Make toJSON notation configurable #147

Closed
wants to merge 5 commits into from
Closed

Conversation

okko
Copy link

@okko okko commented Oct 30, 2020

  • Add toJSON tests
  • Make toJSON notation configurable

Thank you for developing big.js. We're using it to handle product prices. With prices the exponential notation is actually longer, for example the price of 0.86 became 86e-2 with big.js 6.0.0. So with using Express, res.json({price: new Big('0.86')}) used to output {price: '0.86'} and now it outputs {price: '86e-2'}.

The additional byte would not be a big deal for us, but when the values are exposed as JSON strings, the requesting end would now need to properly handle the values with exponents, and we unfortunately cannot control that.

In order to support the old normal notation with .toJSON, this PR makes it configurable.

% npm test

> big.js@6.0.1 test ./big.js
> node ./test/runner.js


 Testing big.js

 Testing abs... 486 of 486 tests passed in 4 ms
 Testing cmp, eq, gt, gte, lt, lte... 23943 of 23943 tests passed in 57 ms
 Testing div... 5027 of 5027 tests passed in 509 ms
 Testing minus... 1723 of 1723 tests passed in 20 ms
 Testing mod... 1756 of 1756 tests passed in 87 ms
 Testing plus... 1845 of 1845 tests passed in 22 ms
 Testing pow... 1026 of 1026 tests passed in 124 ms
 Testing prec... 277 of 277 tests passed in 2 ms
 Testing round... 5199 of 5199 tests passed in 31 ms
 Testing sqrt... 1236 of 1236 tests passed in 173 ms
 Testing times... 2087 of 2087 tests passed in 42 ms
 Testing toExponential... 395 of 395 tests passed in 2 ms
 Testing toFixed... 941 of 941 tests passed in 4 ms
 Testing toNumber... 65 of 65 tests passed in 1 ms
 Testing toPrecision... 425 of 425 tests passed in 3 ms
 Testing toString... 1012 of 1012 tests passed in 4 ms
 Testing toJSON... 1948 of 1948 tests passed in 8 ms

 In total, 49391 of 49391 tests passed in 1.142 secs.

MikeMcl added a commit that referenced this pull request Oct 31, 2020
@MikeMcl
Copy link
Owner

MikeMcl commented Oct 31, 2020

Your feedback has shown me that it was a mistake to change toJSON to always return exponential notation.

I considered this PR and instead decided to change toJSON to just be an alias of toString, which is the simplest way of handling the issue.

As with toString, whether or not toJSON returns exponential notation is determined by the values of Big.PE and Big.NE as previously.

Thank you for your efforts.

Fixed in v6.0.2.

@okko
Copy link
Author

okko commented Nov 2, 2020

Thank you, this solves our original issue well.

@okko okko closed this Nov 2, 2020
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.

2 participants