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

Game creation form crashes when selecting pyramid variant #163

Closed
merowin opened this issue Jan 5, 2024 · 5 comments · Fixed by #165
Closed

Game creation form crashes when selecting pyramid variant #163

merowin opened this issue Jan 5, 2024 · 5 comments · Fixed by #165
Labels
bug Something isn't working

Comments

@merowin
Copy link
Collaborator

merowin commented Jan 5, 2024

Not sure what exactly causes the error.

Steps to reproduce:
Select pyramid in the variant dropdown in the game creation form -> the form collapses and console shows error "TypeError: t is undefined"

@merowin merowin added the bug Something isn't working label Jan 5, 2024
@benjaminpjones
Copy link
Collaborator

benjaminpjones commented Jan 5, 2024

Ah this is a weird one, thanks for finding it.

I think the root cause is that we construct a game object, in order to get the default config:

export function getDefaultConfig(variant: string) {
  const game = new game_map[variant]( /* no config */);
  return game.defaultConfig();
}

However, instantiating PyramidGo requires a width and a height...

  constructor(config: BadukConfig) {
    this.weights = new Grid(config.width, config.height)
  }

By design, the type system is pretty broken around variants/configs/game_map. I'll need to think more about that. And please let me know if you have thoughts on that as well.

But for the short term fix, I recommend we separate configs from the game instances, since they don't depend on a constructed object. We could potentially stick it in the game_map, something like:

const DEFAULT_BADUK_CONFIG = { width: 19, height: 19, komi: 7 };
const game_map = {
  baduk: { class: Baduk, defaultConfig: DEFAULT_BADUK_CONFIG },
  pyramid: { class: PyramidGo, defaultConfig: DEFAULT_BADUK_CONFIG },
}
export function getDefaultConfig(variant: string) {
  return game_map[variant].defaultConfig;
}

@merowin
Copy link
Collaborator Author

merowin commented Jan 5, 2024

I agree, I think it makes sense that the config (default or not) exists before the game object. At the moment its kindof the other way around (for the default config).

Edit: maybe the method defaultConfig could be static

@JonKo314
Copy link
Collaborator

JonKo314 commented Jan 6, 2024

Edit: maybe the method defaultConfig could be static

I was thinking that too

@benjaminpjones
Copy link
Collaborator

static sounds good, but if I recall, I ran into some issues making an abstract static member: microsoft/TypeScript#34516

Is there a way we can work around that?

@JonKo314
Copy link
Collaborator

JonKo314 commented Jan 6, 2024

Ah, I forgot about that. Also there would be another issue:

Static members cannot reference class type parameters. ts(2302)

Then I'd be fine with your suggestion, @benjaminpjones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants