Add setting to exclude constructors from definition list

[EwenDC]

I am writing a Playwright test suite in TypeScript using VSCode. We are following a standard pattern of using classes wrapped around a Playwright page object that abstract common workflows. For convenience we have created an extremely basic base class that all other classes are extending, which allows us to do inline property definition and omit constructors from the other classes entirely.

export class PageWrapper {
  constructor(protected readonly page: Page) {}
}

export class LoginPage extends PageWrapper {
  readonly email = this.page.getByLabel('Email')
  readonly password =  this.page.getByLabel('Password')
  /* More code here */
}

However, when trying to jump to the class from where we're defining it within test code (like below), VSCode prioritises jumping to the dummy constructor of PageWrapper instead of the actual class of LoginPage (though will still show both in the definitions list).

const loginPage = new LoginPage(page)

I understand the logic behind this behaviour (since this is technically an invocation of the PageWrapper constructor function), but in an ES6+ codebase I think the constructor definition is less important than the class definition. After all, if we needed to see the constructor code in this instance, we can follow the class dependency chain down to the invoked constructor.

I propose a new setting that allows constructors from being excluded from the definitions list when jumping to definition from a constructor invocation. I don't know whether a setting like this is necessary for languages other than JavaScript, or if this behaviour is just a byproduct of how JavaScript internally treats constructors.

[mjbvz]

Possibly could be handled by go to type definition or go to declaration (which VS Code supports but TS doesn't implement)

[RyanCavanaugh]

I don't even think this needs to be a setting. Going to new Foo should take you to class Foo { if it has an implicit constructor; from there you can jump to the base class if needed. @mjbvz agree?

[mjbvz]

Yes I don't think we should add a setting. For an explicit constructor, my first feeling was that go to type definition should jump to the class. Not as sure about implicit/inherited ctors. Let's discuss this at our next sync

[RyanCavanaugh]

Agreed in the VS Code sync to show both the constructor (as the first result) and the class definition (as the second result) when the constructor definition isn't a child of the class definition

[iisaduan]

Update from today's sync-- the current behavior when using goToDefinition on new [|SubClass|]() is to show both the constructor and the subclass definition in the "peek" view. The order shown by VSCode is decided by their respective locations in the file(s). The class definition is the primary definition, if you would like to ignore the constructor altogether, it can be skipped by the setting editor.gotoLocation.multipleDefinitions: goTo. (And because the class definition is the primary definition, editor.gotoLocation.multipleDefinitions: goToAndPeek will also bring you to the class definition, but also shows the constructor in the peek view list)

We will add the class definition for non-extended classes, as currently

class Foo {
    /*
     * possibly a lot of code
     */
    constructor()
}
const foo = new /*goToDefinition*/Foo()

only gives the constructor as the definition.

[EwenDC]

That doesn't align with my experience. I've found in the given example that the superclass constructor always shows above the subclass definition, and setting multipleDefinitions just causes you to always get sent to the constructor and not the subclass.

[iisaduan]

Are you able to share your user settings, more code, or more about your file structure? With just the code given here, I'm consistently getting the jump to the subclass definition, and both results showing in the peek/list view.

The peek/list view ordering is only based off of the locations of the definitions in files.

[EwenDC]

Frustratingly I cannot reproduce in a minimal reproduction, only in our actual codebase. When literally copying and pasting our actual code into codeSandbox, it doesn't even show the constructor in the list of definitions when using jump to.

The only hint I can give as to potential root causes is that our codebase is split into two TypeScript projects. The two config files are tsconfig.json and tsconfig.strict.json. They both live at the root level of the repo.

[iisaduan]

@EwenDC I have tried to repro from this additional information, but I am still unable to. I can look into it further/make a fix if you are able to share some repro, but since everything I've tried offers both the constructor and the class, there's not much else I can do without a concrete case.

[EwenDC]

My issue isn't that it's only showing the constructor or the class, my issue is that it's prioritising jumping to the parent class's constructor instead of the child class's definition. In our internal repository multiple developers have experienced this.

In practical terms, this means that either when we have editor.gotoLocation.multipleDefinitions set to "peek", it shows the constructor at the top of the list. Or when we set it to "goto" it'll always jump straight to the parent class constructor and skip the child class definition

[EwenDC]

Agreed in the VS Code sync to show both the constructor (as the first result) and the class definition (as the second result) when the constructor definition isn't a child of the class definition

This comment makes it sound like my experience is 100% intended, and I disagree with that behaviour strongly. Like I said in my original post, the child class definition is far more relevant most of the time than the parent class constructor.

[iisaduan]

This comment makes it sound like my experience is 100% intended, and I disagree with that behaviour strongly. Like I said in my original post, the child class definition is far more relevant most of the time than the parent class constructor.

I missed the discrepancy between the comment you mentioned here when I was writing up my comment summarizing the following VS Code sync. My update is the corrected version of our intentions: class definition first, constructor second--apologies for not making this more clear.

For some more context, after Sync 1, we found that class definition first, constructor second was already the current behavior, and made sense to keep. So in Sync 2, we decided on https://github.com/microsoft/TypeScript/pull/59421, which adds this same behavior to a previously overlooked case. This means that the behavior you're experiencing is a bug since it sounds like the requested behavior should be happening in the first place

[EwenDC]

Cool - thank you for clarifying, and thank you for addressing the issue 🙂