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

When constructing St.Widget, specifying layout_manager (as a subclass of LayoutManager) should be reflected in types #145

Open
KSXGitHub opened this issue Feb 11, 2024 · 2 comments
Labels
gjs-convention Marks issues for aligning with GJS coding standards and practices

Comments

@KSXGitHub
Copy link

KSXGitHub commented Feb 11, 2024

From gjsify/gnome-shell#18


When one constructs a St.Widget like this:

const myWidget = new St.Widget({
  layout_manager: new Clutter.GridLayout(),
})

The type of myWidget.layout_manager is still LayoutManager, which would make TypeScript errors when one tries to use methods from GridLayout.

Suggestion 1: Multiple type parameters

// St
class Widget<LayoutManagerT extends Clutter.LayoutManager | null = null, LabelActorT extends Clutter.LabelActor | null = null> extends Clutter.Actor<LayoutManagerT, LabelActorT> {
  constructor(config?: Widget.ConstructorProperties<LayoutManagerT, LabelActorT>)
}

// Clutter
interface Actor<LayoutManagerT extends LayoutManager | null, LabelActorT extends LabelActor | null> {
  layout_manager: LayoutManagerT
  layoutManager: LayoutManagerT
  label_actor: LabelActorT
  labelActor: LabelActorT
}

Warning

ConstructorProperties now shouldn't be a single interface, but a union of snake_case properties and camelCase properties. Keeping it as a single interface would have required user to specify both layout_manager and layoutManager, which would be absurd.

Suggestion 2: Type dictionary as a single type parameter

// St
class Widget<Config extends Widget.ConstructorProperties | null> extends ClutterActor<Config> {
  constructor(config: Config)
}

// Clutter
interface Actor<Config extends Widget.ConstructorProperties | null> {
  layout_manager: Config extends Widget.ConstructorProperties ? Config['layout_manager'] : null
  layoutManager: Config extends Widget.ConstructorProperties ? Config['layoutManager'] : null
  label_actor: Config extends Widget.ConstructorProperties ? Config['label_actor'] : null
  labelActor: Config extends Widget.ConstructorProperties ? Config['labelActor'] : null
}

Note

I strongly recommend this solution. ConstructorProperties does not need to change at all.


The two suggestions above still have a flaw: It now requires TypeScript user to pass null explicitly, optional parameter is no longer allowed.

If you want to fix this flaw, then I suggest stop using the class keyword. Instead, you should utilize the TypeScript feature that allows merging variable and type, like so (assuming you choose "suggestion 2"):

const Widget: unknown
  & (new <Config extends Widget.ConstructorProperties> (config: Config) => Widget<Config>)
  & (new (config?: null) => Widget<null>)
interface Widget<Config extends Widget.ConstructorProperties | null> extends Widget.Actor<Config> {}

As for me personally, whether I have to pass null explicitly makes little difference.

@JumpLink JumpLink added the gjs-convention Marks issues for aligning with GJS coding standards and practices label Mar 13, 2024
@JumpLink
Copy link
Collaborator

@KSXGitHub I am not sure if I have understood the problem, could you write sample code to clarify the problem?

@KSXGitHub
Copy link
Author

KSXGitHub commented Mar 13, 2024

I am not sure if I have understood the problem, could you write sample code to clarify the problem?

Short sample code:

const myWidget = new St.Widget({
  layout_manager: new Clutter.GridLayout(),
})

myWidget.layout_manager.attach(myBoxLayout, 0, 0, 7, 1)

Real sample code: Replace the type of _calendar with St.Widget and try using tsc to type check.

https://github.com/KSXGitHub/circular-widgets-gnome-45/blob/91f68d21dda8cf7eb8fa0f27f97340c0a65243da/src/Calendar.ts#L17-L19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gjs-convention Marks issues for aligning with GJS coding standards and practices
Projects
None yet
Development

No branches or pull requests

2 participants