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): new method enumValue #1920

Merged
merged 19 commits into from
Mar 15, 2023
Merged

feat(helpers): new method enumValue #1920

merged 19 commits into from
Mar 15, 2023

Conversation

ocodista
Copy link
Contributor

@ocodista ocodista commented Mar 11, 2023

With this method it is possible to get a random value from an enum.

This is very useful for not having to harcode an enum when generating fake data.

Docs-Preview

grafik

@ocodista ocodista requested a review from a team as a code owner March 11, 2023 20:30
test/__snapshots__/datatype.spec.ts.snap Outdated Show resolved Hide resolved
test/__snapshots__/datatype.spec.ts.snap Outdated Show resolved Hide resolved
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: datatype Something is referring to the datatype module labels Mar 12, 2023
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.

This method closely resembles how helpers.arrayElement and objectValue work, so we should move it to the helpers module instead.

And dont forget to run pnpm run preflight when you are ready.

src/modules/datatype/index.ts Outdated Show resolved Hide resolved
test/datatype.spec.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Mar 12, 2023
test/datatype.spec.ts Outdated Show resolved Hide resolved
test/datatype.spec.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

How will this method work in JavaScript given that enums are typescript-specific?

src/modules/datatype/index.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

How will this method work in JavaScript given that enums are typescript-specific?

Funny answer: That's the funny part, it wont

Technical correct answer:
Enums are converted to JS bloatware code constructs like this:

enum Color { Red, Green, Blue }

var Color;
(function (Color) {
    Color[Color["Red"] = 0] = "Red";
    Color[Color["Green"] = 1] = "Green";
    Color[Color["Blue"] = 2] = "Blue";
})(Color || (Color = {}));

So normally you are BY FAR more better suited with setups like var Color = Object.freeze({ Red: 0, Green: 1, Blue: 2 }), which are not only provide less bloated code, but also performs much better at runtime as they can be optimized by JS engines.
Additionally you are bringing just JS over TS into JS and TS doesn't need to first transpile it to something JS-usable.

Then there are const enums to prevent generating of such bloatware code but it theoretically disables the need in total of enums and you can just use type string literals (like e.g. our SexType). These then just check at compile time if the type literal is suitable for the intended call/usage and has zero runtime overhead.

TL;DR: enums in TS are a total misconcept and failure and Microsoft / the TS Team knows that and communicate that in the past already (I just sadly don't find a source directly for that).

So there is only few really case for using enums (and the should be used carefully):

  • when your programmer audience is not JS native and have also OOP background like Java or C#
  • when you are using them only in your own code and want to over-optimize the readability of your TS code base

When using them in a library (like FakerJS), always ensure they are 100% optional and you can always call the function in a readable easy way like using string literals.

@xDivisionByZerox
Copy link
Member

Funny answer: That's the funny part, it wont

I call BS. An enum as you pointed out is simply converted to a JS object. That means you can use it like a JS object. That means you can use all methods as you would in TS. The catch is that those enum objects need to be handled a little bit differently on our side.

String enums work the same as normal objects would.

// in js
const myObject = Object.freeze({
  FOO: 'bar',
  HELLO: 'world',
});

faker.helpers.objectKey(myObject); // 'FOO' | 'HELLO'
faker.helpers.enumKey(myObject); // 'FOO' | 'HELLO'

faker.helpers.objectValue(myObject); // 'bar' | 'world'
faker.helpers.enumValue(myObject); // 'bar' | 'world'

// in ts
enum MyEnum {
  FOO: 'bar',
  HELLO: 'world',
}

faker.helpers.objectKey(MyEnum ); // 'FOO' | 'HELLO'
faker.helpers.enumKey(MyEnum); // 'FOO' | 'HELLO'

faker.helpers.objectValue(MyEnum ); // 'bar' | 'world'
faker.helpers.enumValue(MyEnum); // 'bar' | 'world'

Numeric enums are working (as you pointed out) a bit differently. You have to know this when working with enums (which sucks, no doubt). So while not working as expected, you can make them work!

// in js
const myObject = Object.freeze({
  FOO: 0,
  BAR: 1,
  // here is the catch
  0: 'FOO',
  1: 'BAR',
});

faker.helpers.objectKey(myObject); // 'FOO' | 'HELLO' | 0 | 1
faker.helpers.enumKey(myObject); // 'FOO' | 'HELLO'

faker.helpers.objectKey(myObject); // 'FOO' | 'HELLO' | 0 | 1
faker.helpers.enumValue(myObject); // 0 | 1

// in ts
enum MyEnum {
  FOO: 0,
  BAR: 1,
}

faker.helpers.objectKey(MyEnum); // 'FOO' | 'HELLO' | 0 | 1
faker.helpers.enumKey(MyEnum); // 'FOO' | 'HELLO'

faker.helpers.objectKey(MyEnum); // 'FOO' | 'HELLO' | 0 | 1
faker.helpers.enumValue(MyEnum); // 0 | 1

By providing the helper functions we essentially provide a bridge that allows users to handle enums as they expect.

[...] but also performs much better at runtime as they can be optimized by JS engines.

Source?
While the JS code IS bloaty, it's just an IIFE that assigns the object keys and values. Why should that be sooo underperformed in contrast to "plain JS object creation and freezing"? Unless you talking about some nanosecond BS again...

@Shinigami92
Copy link
Member

@xDivisionByZerox please be careful in not getting personally and callout terms like "BS"

Yes indeed I'm talking about nano-seconds performance improvements. But also even on a code readability perspective it's possible to confuse folks with ordinal enums more then it helps. string literal types on the other hand are understandable, have no hidden side-effects like getting converted to ordinals in the background and other stuff.

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Mar 12, 2023

@xDivisionByZerox please be careful in not getting personally and callout terms like "BS"

You gave out straight-up wrong information. Why am not allowed to call that out? I wasn't calling YOU out but the information you gave.

Edit:
Oh, I see. You probably are referring to "nanosecond BS". I get it might be a strong word in that context, but nevertheless is negligible.
I still apologize if you felt insulted by that.

Edit2:
"straight-up wrong information" => "Funny answer: That's the funny part, it wont"

@matthewmayer
Copy link
Contributor

Could one extend objectKey/objectValue instead to specifically handle the case of it being passed a TS enum?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 12, 2023

Could one extend objectKey/objectValue instead to specifically handle the case of it being passed a TS enum?

This would degrade the usability of those methods for user defined objects, that actually use numeric keys. So no.

@matthewmayer
Copy link
Contributor

This would be the only Faker method which requires typescript to call into right? I think that would be worthy of calling out in the documentation for this function if accepted.

@ST-DDT
Copy link
Member

ST-DDT commented Mar 12, 2023

This would be the only Faker method which requires typescript to call into right? I think that would be worthy of calling out in the documentation for this function if accepted.

AFAICT this will also work with plain json object pseudo enums (it is basically the same as objectValue while ignoring numeric keys).

faker.helpers.enumValue(Color);

Works for both:

enum Color { Red, Green, Blue }

and

const Color = { Red: 0, Green: 1, Blue: 2 }

And still provides a minuscule readability improvement over objectValue by conveying the given value is an "enum" instead of just some magic object.

We might want to add a hint that it ignores numeric keys though (and add an @see link to objectValue).

@ocodista ocodista requested review from ST-DDT and Shinigami92 and removed request for ST-DDT March 12, 2023 20:15
@ocodista ocodista changed the title feat(enum): Add random selector of enum value feat(enum): Add random selector for enum value Mar 12, 2023
@ocodista ocodista requested review from ST-DDT and removed request for Shinigami92 March 12, 2023 21:58
@ocodista ocodista requested a review from ST-DDT March 14, 2023 18:57
ST-DDT
ST-DDT previously approved these changes Mar 14, 2023
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.

One last change, then this is good to go IMO.

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Mar 14, 2023
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.

I sadly identified some TypeScript problems: TS-playground

So while the runtime code is okay, the actual return type of the function is not real-world usable and therefore lacks the usability for what the function is originally planned for.

Right now I didn't get the types managed myself, hopefully someone of you have more luck.

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.

Those type errors can be fixed with the following changes.
It slightly reduces the DX for the return types of String enums.

@Shinigami92 Any suggestions on how to modify the tests to catch the type errors (for future refactorings)?

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
ocodista and others added 2 commits March 14, 2023 20:40
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@ocodista ocodista requested review from ST-DDT and Shinigami92 and removed request for ST-DDT March 15, 2023 01:34
ocodista and others added 2 commits March 15, 2023 13:30
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@Shinigami92
Copy link
Member

@matthewmayer please also approve or write a new review comment

@Shinigami92 Shinigami92 changed the title feat(helpers): Add random selector for enum value feat(helpers): new method enumValue Mar 15, 2023
@Shinigami92 Shinigami92 merged commit f2abf8b into faker-js:next Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants