What is the logic behind when we use Tile and when we use Tile.prototype?

[etodanik]

export class Tile extends defineHex({
  orientation: Orientation.POINTY, // pointy orientation
  offset: -1, // odd rows are offset
}) {
  // ...
}

When we do new Grid(Tile... we just use Tile, but in some places like distance() we use Tile.prototype which seems to be the legacy approach. Is this by design and I'm missing some logic / motivation? Or is this "technical debt"?

[flauwekeul]

It is by design, but I understand the confusion. I could change distance() (and other functions that require the prototype) to just accept a Hex class/constructor. Then internally, the function can get the prototype property. However, the function doesn't need the class/constructor, it needs the prototype. But from a user perspective having to pass Tile.prototype may be weird... What do you suggest?

[etodanik]

So now that I'm paying it some second thought. I like that it just needs two quite static things, orientation and offset. I think that using Tile everywhere would be a downgrade in testability, developer experience e.t.c

However, passing prototype anywhere is quite weird, as well. I think I know where the source of weirdness is, give me a few minutes I'll prepare a minimalistic example.

[etodanik]

So here are my thoughts in a Playground Link

If you run it, you'll see what I mean.

The tldr; of it is that we keep a simple { orientation, offset} type on the input of stuff like distance() (basically a Partial<> of HexSettings?) but instead of passing around prototypes, we create a static getter getHexSettings(), and instead of clever scope stuff for prototypes, defineHex now simply implements some static readonly stuff for our abstract class Hex. Those static thingies can become abstract as well when the feature lands in the TS language, but it all works already.

Edit:
Ah, I forget that this is pure JS =) Haha
Would that be negotiable?

Let me see if I can make a little sandbox of something similar on pure JS. The tldr; is using prototypes directly is super strange & I never had to do it much anywhere as of late.

And it becomes even more strange when consuming this lib in any TypeScript environment.

Edit 2:
Here's the same in pure JS with a bit less fancy stuff like making Hex into abstract Hex, but just as easily avoiding passing around prototypes.

And you can still define the output of getHexSettings() as returning HexSettings and then require Partial<HexSettings> or just offset/orientation in various functions.

class Hex {
  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static
  // supported in all evergreen browsers
  static orientation; 
  static offset;

  static getHexSettings() {
    return {
      orientation: this.orientation,
      offset: this.offset,
    };
  }
 
  // .. lots of other cool stuff
}


function defineHex(hexOptions) {
  const { orientation, offset } = {
    /* ...defaultHexSettings, */ ...hexOptions,
  };

  return class extends Hex {
    static orientation = orientation;
    static offset = offset;
  };
}

const Tile = defineHex({
  orientation: "POINTY",
  offset: -1,
});

console.log(Tile.orientation); // "POINTY"
console.log(Tile.offset); // -1
console.log(Tile.getHexSettings()); // { offset: -1, orientation: "POINTY" }

[flauwekeul]

Just a quick reply because I don't have time: how would you get orientation for example from the instance? Tile.constructor.orientation is possible, but I think that's even more weird than using Tile.prototype.

I don't think hex settings are static. They're shared by all instances, that's why they're in the prototype.

[etodanik]

Yes, you're right: Tile.constructor.orientation is also awkward.

But essentially what you have right now is getters that get their scope not even from the class itself but from the scope of the function that created the class.

I think there are two separate problems here:

• Semantics: You don't really want Hex or Tile in distance(). distance() is a mathematical pure function and all it wants is some HexSettings the fact that the prototype satisfies that is purely coincidental IMHO. It somehow bothers me - and it's a bit clunkier to work around when consuming the lib. HexSettings are easier to serialize, pass around, store, send over the wire, e.t.c - You can't do any of that with a prototype. Of course, we need an easy way to also pass something from Tile to make distance know what the settings are, but IMHO something like Tile.getSettings() would still be a good idea. Right now if I simply decide to pass a perfectly fine object with { offset, orientation } to distance it will swear at me in Typescriplish: Argument of type '{ offset: -1; orientation: Orientation; }' is not assignable to parameter of type 'Pick '. Object literal may only specify known properties, and 'orientation' does not exist in type 'Pick '.
• Implementation, how do we implement the right semantics? That's a very mechanical question, it can probably have many possible answers.

Now - if we can agree on the semantics, I think we could come up with solutions. But do we agree on the semantics?

To me as an end user this lib keeps feeling weird, and the source of this weirdness many times is because of passing either Tile around, or its' prototype to functions that seemingly don't REALLY need it. I'd much rather pass around a serializable HexSettings object than a prototype around.

Also for testing, I have loads of use cases where I'd love to do:
toCube(settings, hex) where settings are a simple HexSettings object. It feels unnecessary to be yelled at by TypeScript that I should have given it a prototype (when literally no unique property of a prototype is needed. Just the settings)

[flauwekeul]

We agree on the semantics 🙃 I'll need some time to think about this. I've experimented a lot in earlier versions with this and the current solution with the prototype was, then, the best of the worst. But I agree it's still a bit meh.

[flauwekeul]

A relatively simple solution would be to change the functions that require (part of) the prototype to just require hex settings. It seems these functions are: pointToCube(), offsetToCube(), hexToOffset() (this actually needs a hex instance), toCube() and distance() (or did I miss any?).

However, that would only change those functions' signatures, but you'd still have to pass either a hex instance or Hex.prototype. I could add a static getter settings that can be used like so:

class Tile extends Hex {
  get dimensions(): Ellipse {
    return createHexDimensions(30)
  }

  get orientation(): Orientation {
    return Orientation.FLAT
  }
}

Tile.settings
// {
//   dimensions: { xRadius: 30, yRadius: 30 },
//   offset: -1,
//   orientation: "FLAT",
//   origin: {x: 0, y: 0}
// }

distance(Tile.settings, [0, 0], [10, 10])

Would this remove some (or all) weirdness in using Honeycomb for you?

[etodanik]

Yes that’d be lovely I think. It’d make the DX a lot more pleasant

…

On 3 Sep 2022 Sat at 14:58 Abbe Keultjes ***@***.***> wrote: A relatively simple solution would be to change the functions that require (part of) the prototype to just require hex settings. It seems these functions are: pointToCube(), offsetToCube(), hexToOffset(), toCube() and distance() (or did I miss any?). However, that would only change those functions' signatures, but you'd still have to pass either a hex instance or Hex.prototype. I could add a static getter settings that can be used like so: class Tile extends Hex { get dimensions(): Ellipse { return createHexDimensions(30) } get orientation(): Orientation { return Orientation.FLAT }} Tile.settings// {// dimensions: { xRadius: 30, yRadius: 30 },// offset: -1,// orientation: "FLAT",// origin: {x: 0, y: 0}// } distance(Tile.settings, [0, 0], [10, 10]) Would this remove some (or all) weirdness in using Honeycomb for you? — Reply to this email directly, view it on GitHub <#86 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPBB6HG2IUKBN54NQ3Q3ZTV4M4O3ANCNFSM6AAAAAAQBIHTNM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[github-actions]

🎉 This issue has been resolved in version 4.0.0-beta.8 🎉

The release is available on:

• GitHub release
• npm package (@beta dist-tag)

Your semantic-release bot 📦🚀