MikeMcl / bignumber.js

A JavaScript library for arbitrary-precision decimal and non-decimal arithmetic
http://mikemcl.github.io/bignumber.js
MIT License
6.63k stars 743 forks source link

[Bug] Return Type of Methods #368

Open ozum opened 5 months ago

ozum commented 5 months ago

Hi,

Thanks for this great library.

I'm developing a subclass by extending BigNumber to add my custom utility methods. However, when I tried to access superclass methods, I got type errors from TypeScript. (The code works as expected; I just got TypeScript typing errors).

class ExtendedNumber extends BigNumber {
  foo(): number {
    return 1000;
  }

  bar(x: number): this {
    const c = this.plus(x);
    const d = c.foo(); // <--------------- ERROR: Property 'foo' does not exist on type 'BigNumber'. ts(2339)
    return this;
  }
}

const value = new ExtendedNumber("10.1");
console.log(value.foo());
console.log(value.bar(1));

The problem is that the return type of methods is hard-coded as BigNumber. They should be this instead of BigNumber.

For example see the original code from bignumber.d.ts file below:

plus(n: BigNumber.Value, base?: number): BigNumber;

It should be:

plus(n: BigNumber.Value, base?: number): this;

Kind Regards,

ozum commented 5 months ago

After working for a while, I realized the problem was deeper than I thought. The code contains 56 hard-coded new BigNumber() calls.


```class ExtendedNumber extends BigNumber {
  bar(): this {
    return this.add(1).decimalPlaces(2);ts(2339)
  }
}

let value = new ExtendedNumber("10.1"); // value is instance of ExtendedNumber
value = value.bar(); // value is instance of BigNumber !!!!!

I tried to replace all new BigNumber() with new this.constructor(). However, in some places, this is not available, so I tracked the calling code and replaced, for example, parseNumeric(...) with parseNumeric.call(this, ...) to make this available in the parseNumeric() function.

Unfortunately, when I changed maxOrMin.call(this, ...), tests failed to the degree that I needed to investigate much more deeply, and I couldn't continue.

In the current situation, BigNumber.js seems hard to extend in a subclass. I'll be happy if you can change that, but it may need much more effort than you want to spend.

Nonetheless, this is a very good library.

Kind regards,

ArtemKolichenkov commented 3 weeks ago

Hi! BigNumber.js doesn't work the way you think it does.

Pretty much every method you call on an instance of BigNumber (e.g. .plus(), .minus(), .multipliedBy, etc) does not modify the original instance, it returns a brand new instance. Each instance is effectively immutable, which is on purpose, it prevents you from writing buggy code (for example - if you have some global constant BigNumber - you can safely do all kinds of stuff with it and don't worry about mutating it accidentally).

So the return type of a method is actually never this, it is always BigNumber (unless of course it's something else like with .toFixed(), isEqualTo(), etc).

If you really want to do return this in your extended class methods you will have to work around that by creating a new instance of ExtendedNumber when you need it and the continue to use your custom methods on it.

bar(x: number) {
    const c = new ExtendedNumber(this.plus(x));
    const d = c.foo();
    return this;
  }

I would strongly discourage this though, it's way easier to overlook something and write buggy code this way.

It might seem like constantly creating instances is a waste of memory and CPU cycles, but:

  1. This is JavaScript, your app probably eats up 1000x memory in many other parts of the code, optimize those parts, not this.
  2. Unless you're handling literally billions of BigNumber objects every second - whatever you're doing with BigNumber probably takes <1ms to execute, even with constant creation of new objects.