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

feat(helpers)!: stricter checking for function signature passed to multiple #2563

Merged
merged 9 commits into from
Mar 3, 2024

Conversation

pomali
Copy link
Contributor

@pomali pomali commented Nov 30, 2023

faker.helpers.multiple() supports using mapFn like in Array.from(), but type signature of function didn't match, PR fixes this and adds test for this case

(but since it changes declared interface I chose feat instead of fix as semantic type)

@pomali pomali requested a review from a team as a code owner November 30, 2023 10:11
@xDivisionByZerox
Copy link
Member

I'm not sure if this fits the use case of faker.helpers.multiple 🤔

If you require the parameters of the mal function, you are most likely doing some static value generation.

I need some time to think about this in more detail.

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent m: helpers Something is referring to the helpers module labels Nov 30, 2023
@xDivisionByZerox xDivisionByZerox added this to the vAnytime milestone Nov 30, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team November 30, 2023 10:40
@pomali
Copy link
Contributor Author

pomali commented Nov 30, 2023

Yes, usecase here is generating two sets of objects with matching ids, so I can "join" them later.
Eg.

interface School { locationId: number }
interface Location { id: number }

I could create an function with no arguments that simulates this behavior, but it seemed busywork since underlying code already supports it.

I am using it in place without shared state (in mock service worker in two different endpoints)

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The index parameter might be useful when generating a list of entries that should have sequential ids.

However I would like to block this until v9.0 development commences, because then the method might get a new parameter (ref fakerCore) anyway.

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the s: on hold Blocked by something or frozen to avoid conflicts label Nov 30, 2023
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.56%. Comparing base (9348138) to head (529aac6).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2563      +/-   ##
==========================================
- Coverage   99.57%   99.56%   -0.02%     
==========================================
  Files        2846     2846              
  Lines      248715   248717       +2     
  Branches      655     1011     +356     
==========================================
- Hits       247669   247634      -35     
- Misses       1017     1083      +66     
+ Partials       29        0      -29     
Files Coverage Δ
src/modules/helpers/index.ts 98.82% <100.00%> (+<0.01%) ⬆️

... and 30 files with indirect coverage changes

@xDivisionByZerox xDivisionByZerox modified the milestones: vAnytime, v9.0 Dec 10, 2023
@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision s: accepted Accepted feature / Confirmed bug and removed s: on hold Blocked by something or frozen to avoid conflicts s: needs decision Needs team/maintainer decision labels Feb 9, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Feb 15, 2024

Sorry for the delay in feedback.
Could you please update this PR with the latest suggestions?

Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@pomali
Copy link
Contributor Author

pomali commented Feb 20, 2024

Sorry for the delay in feedback. Could you please update this PR with the latest suggestions?

No problem, sorry for mine delay. Updated.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 21, 2024

Could you please fix the broken tests and examples?
The tests were already broken due to the wrong typing of the method before, but now the method actually requires proper types and thus the bad usage actually shows up.

firstName -> () -> firstName()

@ST-DDT
Copy link
Member

ST-DDT commented Feb 21, 2024

Please also enable allow edits by maintainers on this PR so we can update it ourself to the latest commits on next.

test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT dismissed stale reviews from Shinigami92 and themself via ac8724d March 1, 2024 23:37
ST-DDT
ST-DDT previously approved these changes Mar 1, 2024
@ST-DDT ST-DDT requested review from Shinigami92, a team and rtm-ctrlz and removed request for rtm-ctrlz March 1, 2024 23:40
Copy link

@rtm-ctrlz rtm-ctrlz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub still asking me for review :)

In my opinion this change should be accepted and marked as breaking change.

In most cases this change should not lead to breaks, but it worth a notice.

docs/guide/upgrading_v9/2563.md Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Show resolved Hide resolved
docs/guide/upgrading_v9/2563.md Outdated Show resolved Hide resolved
@ST-DDT ST-DDT changed the title feat(helpers)!: expose signature parameters in method parameter of multiple feat(helpers)!: stricter checking for function signature passed to multiple Mar 2, 2024
ST-DDT
ST-DDT previously approved these changes Mar 2, 2024
@ST-DDT ST-DDT requested review from matthewmayer and a team March 2, 2024 12:41
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Matt Mayer <152770+matthewmayer@users.noreply.github.com>
@ST-DDT ST-DDT requested a review from matthewmayer March 2, 2024 13:38
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from a team March 2, 2024 14:36
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good migration guide 👍

@ST-DDT ST-DDT merged commit 2b15f2e into faker-js:next Mar 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants