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

helpers.multiple fails with a generic error when the number of elements exceeds 2^32 - 1 #2374

Closed
9 of 10 tasks
ST-DDT opened this issue Sep 3, 2023 · 4 comments
Closed
9 of 10 tasks
Assignees
Labels
c: bug Something isn't working c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: helpers Something is referring to the helpers module m: string Something is referring to the string module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Sep 3, 2023

Pre-Checks

Describe the bug

When calling faker.helpers.multiple() with a value of or over 2^32 the method will fail with a generic error.

RangeError: Invalid array length
    at Function.from (<anonymous>)
    at V.multiple (file:///.../faker/dist/esm/chunk-5MJ2XURO.mjs:13:698)

Minimal reproduction code

faker.helpers.multiple(() => 1, { count: Number.MAX_SAFE_INTEGER });

Additional Context

Created after some testing for #2372

This issue is NOT about handling the absence of a required parameter.

Environment Info

System:
    OS: Windows 10 10.0.22621
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz       
    Memory: 5.91 GB / 15.78 GB
  Binaries:
    Node: 18.17.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD
    npm: 8.12.2 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.1.1 - C:\Program Files\nodejs\pnpm.CMD
  Browsers:
    Edge: Spartan (44.22621.2215.0), Chromium (116.0.1938.69)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @faker-js/faker: 8.0.2 => 8.0.2

Which module system do you use?

  • CJS
  • ESM

Used Package Manager

pnpm

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module m: string Something is referring to the string module labels Sep 3, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 3, 2023

We have to decide on how to proceed here:

  1. Should we limit the max number of array elements?
  2. To which value should we limit the number of elements to?
  3. By throwing an error or limiting the length
  4. faker.string.sample() has a limit (2^20), all other string methods don't, should we apply the same limit everywhere?

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Sep 3, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 10, 2023

Team Decision

  • Revert limit on 2^20 for faker.string.sample()

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs and removed s: needs decision Needs team/maintainer decision labels Oct 10, 2023
@ST-DDT ST-DDT modified the milestones: v8.x, vAnytime Oct 10, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 22, 2023

Created PR #2497 to address this.

@ST-DDT ST-DDT self-assigned this Oct 22, 2023
@xDivisionByZerox
Copy link
Member

I will close this issue. #2497 has been merged and was first release in https://github.com/faker-js/faker/releases/tag/v8.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: helpers Something is referring to the helpers module m: string Something is referring to the string module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

No branches or pull requests

2 participants