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

array-like elements have default arguments which violate genericity #219

Closed
damienwebdev opened this issue Jan 18, 2022 · 8 comments · Fixed by #2045
Closed

array-like elements have default arguments which violate genericity #219

damienwebdev opened this issue Jan 18, 2022 · 8 comments · Fixed by #2045
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@damienwebdev
Copy link
Member

Describe the bug

I was reviewing https://github.com/faker-js/faker/pull/189/files and I noticed that randomize supports generic results, but there's an issue with the arguments having a default.

Consider:

const a = randomize<number>();

This would return a string, which violates the type.

Reproduction

TypeScript Playground Link

Additional Info

No response

@damienwebdev damienwebdev added the s: pending triage Pending Triage label Jan 18, 2022
@Shinigami92
Copy link
Member

I assume the only way to really fix this, is to fully remove the default and force an argument to be passed in.
This would be a breaking change.

@poyea
Copy link

poyea commented Jan 18, 2022

It seems the violation happens when the argument is undefined / empty. Are we trying to achieve

const a: number = randomize<number>();
console.log(a); // 1

const b = randomize();
console.log(b); // "a"

or else we may have to think of other ways

@Shinigami92
Copy link
Member

It seems the violation happens when the argument is undefined / empty. Are we trying to achieve

const a: number = randomize<number>();
console.log(a); // 1

const b = randomize();
console.log(b); // "a"

or else we may have to think of other ways

Yeah, but sadly this is not possible due to TS is only at compile time.
So the generic cannot change runtime behavior 🤷
So we may just remove the default value for the parameter and then declare it as breaking change

@poyea
Copy link

poyea commented Jan 18, 2022

It seems the violation happens when the argument is undefined / empty. Are we trying to achieve

const a: number = randomize<number>();
console.log(a); // 1

const b = randomize();
console.log(b); // "a"

or else we may have to think of other ways

Yeah, but sadly this is not possible due to TS is only at compile time. So the generic cannot change runtime behavior 🤷 So we may just remove the default value for the parameter and then declare it as breaking change

Yes if we've to change it. At least, generic specialization in TS is not possible.

@ejcheng ejcheng added c: bug Something isn't working and removed s: pending triage Pending Triage labels Jan 18, 2022
@Shinigami92 Shinigami92 added this to the v6.1 - First bugfixes milestone Jan 28, 2022
@ejcheng ejcheng moved this to Todo in Faker Roadmap Feb 12, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Mar 23, 2022

This function is marked as deprecated (for removal in v7) and we already want to get rid of the default ['a', 'b', 'c'] (#589)
So I'm not sure if it's worth working on this issue specifically

Maybe we should just close this issue?

@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Mar 23, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 23, 2022

Isn't #589 the fix for this issue?

@Shinigami92 Shinigami92 linked a pull request Mar 24, 2022 that will close this issue
@Shinigami92 Shinigami92 moved this from Todo to Awaiting Review in Faker Roadmap Mar 24, 2022
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent and removed s: needs decision Needs team/maintainer decision labels Mar 24, 2022
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug breaking change Cannot be merged when next version is not a major release labels Sep 8, 2022
@xDivisionByZerox
Copy link
Member

Blocked by #893

@xDivisionByZerox xDivisionByZerox added the s: on hold Blocked by something or frozen to avoid conflicts label Jan 29, 2023
@xDivisionByZerox xDivisionByZerox moved this from Awaiting Review to Todo in Faker Roadmap Jan 29, 2023
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Apr 13, 2023
@Shinigami92
Copy link
Member

Shinigami92 commented Apr 13, 2023

Team Decision

Can be implemented without #893

We want this for v8.0

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: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
6 participants