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

[Bug] Return Type of Methods #368

Open
ozum opened this issue Mar 12, 2024 · 2 comments · May be fixed by #369
Open

[Bug] Return Type of Methods #368

ozum opened this issue Mar 12, 2024 · 2 comments · May be fixed by #369

Comments

@ozum
Copy link

ozum commented Mar 12, 2024

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 added a commit to ozum/bignumber.js that referenced this issue Mar 12, 2024
The hard-coded return type "BigNumber" prevents subclasses from working correctly with TypeScript types when super class methods are called.

See MikeMcl#368, this PR closes MikeMcl#368
@ozum ozum linked a pull request Mar 12, 2024 that will close this issue
@ozum
Copy link
Author

ozum commented Mar 12, 2024

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
Copy link

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.

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 a pull request may close this issue.

2 participants