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

Constructor fails to recognize valid input #21

Open
FrozenKiwi opened this issue Mar 15, 2022 · 2 comments
Open

Constructor fails to recognize valid input #21

FrozenKiwi opened this issue Mar 15, 2022 · 2 comments

Comments

@FrozenKiwi
Copy link

We can make the constructor a lot more robust by removing the value instanceof Decimal type guard.

The problem with this guard is that you can get false negatives (esp if you end up with different versions of Decimal in your project, for whatever reason). This causes very difficult-to-trace issues where code will throw on valid inputs depending on where the source object was constructed.

The condition probably doesn't need to be quite so restrictive. What if it was replaced with duck-typing? (this could also help in dumping to/from json).

if (typeof value.s == 'number' && typeof value.e == 'number' ... etc )
@MikeMcl
Copy link
Owner

MikeMcl commented Mar 15, 2022

And then the trickier one you missed out: checking if value.d is an array using ECMAScript 3 features only (or just use typeof value.d == 'object', of course).

It's quite a lot of duck-typing on a very hot code path, for the probably rare case when for some reason there are multiple versions of Decimal present (which could in any case be handled albeit less efficiently by calling toString on their instances when passing them to a different Decimal's methods).

Anyway, you make some fair points, and I will leave this issue open for consideration. Thanks for your input.

@FrozenKiwi
Copy link
Author

Thanks for the consideration. I'd be happy to contribute if you can think of something acceptable (I'm aware that I'm probably in a small minority with this issue!)

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

No branches or pull requests

2 participants