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

Support parameter types for mixin constructors #28232

Closed

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

@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

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
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.

3 participants