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

randFloat with equal min/max results in RangeError: Maximum call stack size exceeded #361

Open
soanvig opened this issue Aug 6, 2023 · 6 comments

Comments

@soanvig
Copy link

soanvig commented Aug 6, 2023

Is this a regression?

No

Description

If I call randFloat({ min: 0, max: 0 }) I get maximum call stack size exceeded error.

  1. The same happens for other min/max values that are equal
  2. Probably other number generators might be affected as well.

This should result in 0, since 0 satisfies min and max requirements.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

Note I'm using yarn PnP

RangeError: Maximum call stack size exceeded

  › l (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:442)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115474)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)
  › e (.yarn/cache/@ngneat-falso-npm-6.4.0-d3fcdd6e1d-b7711c0418.zip/node_modules/@ngneat/falso/index.cjs.js:1:115553)

Please provide the environment you discovered this bug in

No response

Anything else?

No response

Do you want to create a pull request?

Yes

@shaharkazaz
Copy link
Contributor

shaharkazaz commented Aug 23, 2023

@soanvig Well, generally speaking, this is kind of a misuse, but I do agree that this shouldn't result in a stack overflow.
I have some code lying around that should prevent this in TS level by preventing you from passing this kind of configuration, I can add it.
But we still need to support cases where this isn't consumed as part of a TS project so if you want to open a PR for it go ahead 🙂

@soanvig
Copy link
Author

soanvig commented Aug 23, 2023

@shaharkazaz this is definitely not misuse. Min/max range is inclusive. Therefore min: X, max: X should produce X, because X∈<X;X>

@shaharkazaz
Copy link
Contributor

@soanvig can you share your use case where you use a random range function where the minimum and maximum are the same and not just use the value itself?

From the use cases I've encountered with Falso I never needed anything like that that's why I'm asking :)

@soanvig
Copy link
Author

soanvig commented Aug 23, 2023

I don't see the reason to give the use case, because that's clearly a bug if there is inconsistency in how Falso works.

I use falso in production code actually. I'm writing a library that generates random data, including numbers.

  1. One case is that I receive min/max range from the libary's user input
  2. Second case is that I simply do some calculations before using falso, and for whatever reason these calculations finish with the result min=max, which is perfectly valid.

I have nothing against falso not working for min=max, but it simply doesn't make sense, and is misleading, because min and max are inclusive ranges (telling from the results produced by Falso). Even if different error is thrown it's still incosistency, and honestly I don't see any reason to not support min=max.

@shaharkazaz
Copy link
Contributor

shaharkazaz commented Aug 23, 2023

@soanvig

I don't see the reason to give the use case, because that's clearly a bug

It's okay to have a conversation over things, I find the tone of your comment a bit aggressive even though there is no real reason for it. you could have just dropped the intro and written whatever you wrote anyway, I was just interested in your use case. You and I both want to make this OS better.

You are welcome to open a PR to support use cases where the min & max inputs are equal and return this value, make sure you make the change across all functions (if we have any more cases like this)

@soanvig
Copy link
Author

soanvig commented Aug 23, 2023

@shaharkazaz don't worry, I'm never aggressive. After all I answered the question. It's just I feel that the if there is a bug (I think we all agree incosistency is a bug), there is a bug, and talking about use cases dilutes the conversation and diverges from the main topic. Probably if you answered "yeah, that's OK if you open PR", and then asked about use cases out of curiosity, then I wouldn't add that line. That's all :)

I plan to send PR in 2 weeks then.

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