Fix: Orientation being used as type and value

[vfonic]

Orientation is a TS type (it's actually an enum), but in some places it's used as a string value: orientation === Orientation.POINTY

You probably want to compare that orientation equals 'POINTY' string here, but this is comparing that orientation is of type 'POINTY' string. It's pretty-much the same thing, but just seems slightly off.

This could cause issues, because, after the TS => JS compilation, there's no more TS type Orientation. A better approach is to create an Orientations const that can be used in TS as well as in JS, as a value. To get the type of such const, you can use typeof Orientations.

To say that some value needs to be one of the Orientations values, you can write: ValueOf<typeof Orientations>, where type ValueOf<T> = T[keyof T]

This change also fixes the use of 'flat', 'FLAT', 'pointy' and 'POINTY' to allow only uppercase versions: 'FLAT' and 'POINTY'.

I don't see the need to accommodate both 'FLAT' and 'flat'. We could also accommodate 'fLaT' and 'not pointy', etc. I think allowing only one value clears up the API. It's good that you're working on the next version so it's a perfect time to introduce such breaking changes.

as const: https://blog.logrocket.com/const-assertions-are-the-killer-new-typescript-feature-b73451f35802/ ValueOf: https://stackoverflow.com/questions/49285864/is-there-a-valueof-similar-to-keyof-in-typescript

[flauwekeul]

First of all: thanks a lot for giving feedback and opening a PR to improve the code 🎉 Let's get into it:

This could cause issues, because, after the TS => JS compilation, there's no more TS type Orientation.

Actually, there is. And I don't see any issues with the current way of using the enum. orientation === Orientation.FLAT works just fine.

I don't see the need to accommodate both 'FLAT' and 'flat'.

I think it's a small convenience to let people also supply the lowercase version. Mainly for JS users it's annoying to have the lib not working if you forget to type the value for orientation in uppercase. Having a more forgiving API (that also works well for people who don't use TypeScript) is more important than having a "cleaner" (?) API.

Although I appreciate the effort you put into making this PR, I'm not convinced (yet) I should merge it...

[vfonic]

What do you suggest?

There's an issue we have in the existing codebase that we cannot use string values such as 'POINTY' because typescript complains. In my solution it works, but we need two separate entities: a type (Orientation) and a value (Orientations).

EDIT: I removed the src-js changes, squashed the commits into one and force-pushed

[vfonic]

Allowing users to use both 'flat' and 'FLAT' adds a big issue in terms of the project code. I'd argue that, although it's great to improve the DX, it just costs too much for the little added value.

You'll have to create a "sanitizer" of sorts that will not be able to assume orientation type is Orientation (for example Orientation.FLAT), but it will be something of the kind: Orientation.FLAT | Orientation.flat or 'FLAT' | 'flat', Orientation | OrientationLowercase etc.

I think it's better to do the conversion when receiving this argument, rather than somewhere down the road, where you might forget, where you have to do it any time you want to get the value, and the error you might get might not make sense. If you get the error immediately when sanitizing, you'll get "wrong argument for orientation", while if you do it later you might get "undefined is not a function", because you'll try to upper/lower case the user-passed value, which could be undefined, but we didn't sanitize it.

[flauwekeul]

Allowing users to use both 'flat' and 'FLAT' adds a big issue in terms of the project code. I'd argue that, although it's great to improve the DX, it just costs too much for the little added value.

I'm not aware of this big issue 😅 . The only place where a regular string ('pointy' or 'flat') is allowed (as well as the enum) is when calling createHexPrototype(). In all other places (inside Honeycomb), only the enum is used. This works because createHexPrototype() converts the string value to the enum. This is basically the "sanitation" you're suggesting. There's a normalizeOrientation() that throws an error if the string orientation can't be converted to the enum.

Could you show some code that reveals the issues you're having?

[vfonic]

I remember I gave up on using enums in TS and I just went to remind myself why. There are quite some blog posts and SO questions where people are suggesting alternatives. I know I always use as const and it works for any use-case that I have.

Here's another interesting solution: https://stackoverflow.com/a/35257367/336806

That's all I know. :) I tried using enums, figured it's broken, and found a solution that works for me in all use-cases.

Here's a simple example of broken enums:

enum Enum {
  LOL = 'lol',
}

type LolType = {
  broken: Enum
}

const konst: LolType= {
  broken: 'lol' // TS2322: Type '"lol"' is not assignable to type 'Enum'.  index.ts(36, 3): The expected type comes from property 'broken' which is declared here on type 'LolType'
}

TS Playground

Either way, is there anything from this PR you're planning on using or should we just close it?

[flauwekeul]

I'm already using strings as enum values.

To be honest, I still don't know what issue your PR is trying to solve. Unless your talking about the possibility to pass a case-insensitive string or an enum for orientation. I've explained why I think that's a good idea and I'm not convinced I should change it.

I haven't seen any concrete problems with the current use of an enum for orientation, so it seems we've discussed this enough. Thanks again for your effort 👍

[vfonic]

FYI this is actually the comment that got me to start working on this PR: https://github.com/flauwekeul/honeycomb/blob/next/playground/index.ts#L12

[flauwekeul]

Ah, that clears things up 😆

I suspect that bug has a different cause. The error suggests a conflict between the two types that createHexPrototype() accepts (which are T extends Hex and HexPrototypeOptions), there doesn't seem to be any issue with the orientation enum.

I've looked into it and fixed the bug 🎉 . createHexPrototype() should accept an object with type Partial<Omit<T, 'dimensions' | 'orientation' | 'origin' | 'offset'> | HexPrototypeOptions>. So basically, everything from T (the custom Hex type) except the props in HexPrototypeOptions.

This will be released in alpha.2.