Skip to content

Support parameter types for mixin constructors#28232

Closed
igelbox wants to merge 1 commit intomicrosoft:masterfrom
igelbox:mixin-constructor-parameters
Closed

Support parameter types for mixin constructors#28232
igelbox wants to merge 1 commit intomicrosoft:masterfrom
igelbox:mixin-constructor-parameters

Conversation

@igelbox
Copy link
Contributor

@igelbox igelbox commented Oct 30, 2018

This PR adds an ability to use strict constructor signatures on mixin classes for #13743 instead of saying just constructor(...args: any[]).

This feature will be useful if the mixin class needs some arguments from the overridden constructor, e.g.:

abstract class Base {
  constructor(s: string = '') {}
}
const mixin = <T extends typeof Base>(base: T) => class extends base {
  constructor(x?: string) {
    super(x);
    // use the "x"
  }
};

class Incompatible {
  constructor(n: number) {}
}
mixin(Incompatible); // Fails to extend from a class with an incompatible constructor

I researched for the reason of requiring the mixin constructor to be constructor(...args: any[]) but didn't manage to find one which can be violated by this PR.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jan 24, 2019

@igelbox is there an issue tracking this change?

@RyanCavanaugh RyanCavanaugh self-assigned this Jan 24, 2019
@igelbox
Copy link
Contributor Author

igelbox commented Jan 29, 2019

@RyanCavanaugh no, there's no such issue.
Must I file one?

I just faced with this drawback during the development process and then I was glad to dig and fix the code. Filing issues isn't what I'm loving in software development)

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 23, 2019

PRs do need issues tracking their changes.

If a PR just appears with some code changes in it, we don't know what's happening: What is the change trying to do? Is that change desirable? What are its goals? Is it going to be a breaking change? What's an example of code that motivates the change in the first place? Is there a better way to accomplish that task? Does the change actually accomplish that specific goal, or does it do something slightly different? Should some other larger change to the compiler be made instead?

There's a reasonably rigorous process for answering those questions for issues. With a PR, we have to work backwards from the code and baseline changes to determine the answers, which is then conflated and commingled with how the code itself works.

So, long story short, please don't put up PRs that don't have an associated issue tracking them. Our process is fairly lightweight here for straightforward things, but we do need contributors to play along in the process to help keep things moving along smoothly. Thanks!

@sandersn
Copy link
Member

sandersn commented Feb 3, 2020

I'm cleaning up our PR backlog and this is one is fairly stale, so I'm going to close it.

@igelbox The next step is to create an issue since mixin types are fairly tricky and require a bit of design discussion, as well as thinking about backward compatibility, before implementation.

@sandersn sandersn closed this Feb 3, 2020
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 21, 2025

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

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

Reviewers

No reviews

Assignees

@RyanCavanaugh RyanCavanaugh

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@igelbox @RyanCavanaugh @sandersn @typescript-bot