Skip to content

Conversation

@ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Oct 14, 2025

Reverts #9815

After additional thought, I am not comfortable with making "negative changes" to our API based on how one current LLM model performs on it.

Basically, having an API that implicitly expects a static member be defined for subclassing to work is what I consider surprising to end users and such "magic" is not something I would like to find in any API I am using.

I expect that we will need to follow up with discussion in slack. Perhaps someone actually prefers this design and then that is of course a different discussion.

@coveralls
Copy link

Coverage Status

coverage: 91.144%. remained the same
when pulling c033a11 on revert-9815-x/widget-class-2
into 5da7942 on master.

@Pessimistress
Copy link
Collaborator

Our standard code pattern (and in my opinion easier to read) is to merge user props with default in each subclass constructor, like so:

class MyWidget extends Widget {
  constructor(props) {
    super({
      optionA: true,
      optionB: 1,
      ...props
    });
  }
}

The static defaultProps field is used by Layer classes for chained inheritance, which is not needed in the widget system.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Oct 15, 2025

Our standard code pattern (and in my opinion easier to read) is to merge user props with default in each subclass constructor, like so:

Yes, perhaps that is all we need. Compact and explicit.

But can we make this fully typed? And ensure we have all the required props set up? The given example omits the typing...

class Widget<WidgetPropsT> {
  props: Required<WidgetPropsT>;
  constructor(props: Required<WidgetPropsT>) { // do we need to omit WidgetProps here?
    ...
  }
}

class MyWidget extends Widget<MyWidgetProps> {
  constructor(props: MyWidgetProps) {
    super({
      optionA: true,
      optionB: 1,
      ...props
    });
  }
}

Context: The thing that made me most uncomfortable is that we are going around the type system, and rely on a "magic" static property to be declared, i.e. the code below.

 constructor(props: PropsT) {
    this.props = {
      // @ts-expect-error `defaultProps` may not exist on constructor
      ...(this.constructor.defaultProps as Required<PropsT>),
      ...props
    };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants