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

InterfaceDeclarationStructure has no get or set accessors #1431

Closed
ajvincent opened this issue Jul 17, 2023 · 12 comments · Fixed by #1473
Closed

InterfaceDeclarationStructure has no get or set accessors #1431

ajvincent opened this issue Jul 17, 2023 · 12 comments · Fixed by #1473

Comments

@ajvincent
Copy link
Contributor

ajvincent commented Jul 17, 2023

Describe the bug

Version: 19.0.0

To Reproduce

import ts from "ts-morph";
const TSC_CONFIG: ts.ProjectOptions = {
  "compilerOptions": {
    "lib": ["es2022"],
    "module": ts.ModuleKind.ESNext,
    "target": ts.ScriptTarget.ESNext,
    "moduleResolution": ts.ModuleResolutionKind.NodeNext,
  },
  skipAddingFilesFromTsConfig: true,
  useInMemoryFileSystem: true,
};

const project = new ts.Project(TSC_CONFIG);

const sourceFile = project.createSourceFile("file.ts", `
interface Foo {
  get A(): string;
}
`.trim() + "\n");

const decl = sourceFile.getInterfaceOrThrow("Foo");
const structure = decl.getStructure();
console.log(JSON.stringify(structure, null, 2));
{
  "name": "Foo",
  "isExported": false,
  "isDefaultExport": false,
  "hasDeclareKeyword": false,
  "docs": [],
  "typeParameters": [],
  "extends": [],
  "callSignatures": [],
  "constructSignatures": [],
  "indexSignatures": [],
  "methods": [],
  "properties": [],
  "kind": 19
}

ts-ast-viewer link
Expected behavior

I wanted to see the get accessor for A in the output structure.

I believe the fault is in TypeElementMemberedNodeStructure. It lists methods and properties but not get accessors or set accessors.

@ajvincent
Copy link
Contributor Author

ajvincent commented Jul 22, 2023

I can already see there would be a lot to build out.

  • Updating StructureKind
  • GetAccessorSignatureStructure, extrapolated from GetAccessorDeclarationStructure
  • SetAccessorSignatureStructure, extrapolated from SetAccessorDeclarationStructure
  • Implementing GetAccessorSignatureNode with tests
  • Implementing SetAccessorSignatureNode with tests
  • Static methods Node.isGetAccessorSignature(), Node.isSetAccessorSignature()
  • Modifying TypeElementMemberedNodeStructure to include getAccessors and setAccessors properties
  • Adding GetAccessorSignatureStructure and SetAccessorSignatureStructure to TypeElementMemberStructures
  • Updates to the TypeElementMemberedNode interface to reach accessors from a Node
  • Implementing fixes to InterfaceDeclaration::getStructure()
  • Tests for the same
  • Updates to forEachStructureChild and Structure
  • Updating an InterfaceDeclaration from a structure having accessors
  • Tests for the same
  • Documentation in docs/details/interfaces.md

I think briefly about taking this on, but this would be a big chunk of time when I don't have much to spare. I'm writing this comment as a brainstorm attempt at a roadmap, and I'm wondering what I'm missing.

@forivall
Copy link

I have a need for this, so I might collaborate with you on this; i'll let you know if I start on it @ajvincent

@ajvincent
Copy link
Contributor Author

@forivall Sweet. It's always nice to have a motivated collaborator.

I would be quite happy if you took the lead on this. For me, this is more of an annoyance / future need rather than an immediate need. I do have a current clean fork of this repository already, and branches are cheap... so I could invite you as a collaborator there if you'd prefer to use my fork.

I did discover one missed part in my checklist earlier, a few days ago:

Of course, I would still like to get a review of the roadmap to see what else we missed before we dive in too deep.

@ajvincent
Copy link
Contributor Author

ajvincent commented Nov 2, 2023

@forivall I might start on this very soon. That "future need" is getting close. My need is for a "type-to-class" functionality (more accurately a "membered-type-to-class"), and I'm envisioning having the getters and setters defined on the interface before I start running. This would be nicer than modifying the type inside the class builder.

@ajvincent
Copy link
Contributor Author

@dsherret I am formally asking for a design review per CONTRIBUTING.md.

If we do not have enough details here for a proper design, please say so and point me to a good example design you've approved in the past. I want to move forward as soon as practical.

@dsherret
Copy link
Owner

dsherret commented Nov 3, 2023

Adding this sounds good! I think it should be fairly straightforward, but a bit of work.

@ajvincent
Copy link
Contributor Author

@forivall https://github.com/ajvincent/ts-morph/tree/interface-getters-setters just starting out.

@ajvincent
Copy link
Contributor Author

I've just hit the first of probably many stumbling blocks. (I've been busy the last few weeks.) Specifically, I'm looking at building packages/ts-morph/src/compiler/ast/interface/GetAccessorSignature.ts. I'm trying to take inspiration from PropertySignature.ts.

I started by trying to figure out what I need to add to TypeElementMemberedNode.ts to implement a getGetAccessorSignatures() method. I have to use a SyntaxKind that already exists.

The stumbling block is the SyntaxKind enum. I assumed, incorrectly, the get accessors of interfaces would have a different SyntaxKind than those of classes. (Because both SyntaxKind.PropertySignature and SyntaxKind.PropertyDeclaration exist.) Nope, they're both SyntaxKind.GetAccessor.

So I end up with:

    getGetAccessorSignatures() /* : GetAccessorDeclaration[] */{
      return this.compilerNode.members.filter(m => m.kind === SyntaxKind.GetAccessor)
        .map(m => this._getNodeFromCompilerNode(m as ts.GetAccessorDeclaration))
    }

GetAccessorDeclaration already exists.. Great! Except for that "you can't have statements in an interface" part.

So if I can reuse the existing GetAccessorDeclaration class, then maybe this is less work than I thought, but it also means a design rethink.

@dsherret advice please?

@dsherret
Copy link
Owner

@ajvincent would you be able to open a PR with what you've done so far? I'm going to do a major release of ts-morph this weekend probably and can finish this up in case it's not done by then.

It should use GetAccessorDeclaration (see how they're the same here) and not a new node type. GetAccessorDeclaration already has an optional body (it's a BodyableNode and not a BodiedNode). Also see

(probably that should be updated to always write that when writing in an interface)

Thanks for working on this!

@ajvincent
Copy link
Contributor Author

You are most welcome, though I think you'll find I didn't get very deep into this - and I wish I had. I'm pretty good about writing tests and documentation... and I'd be happy to write a follow-up PR for those purposes.

@ajvincent
Copy link
Contributor Author

@dsherret where do we stand on this? I've been battling an illness this week, though it seems to be fading. I'm willing to pick this up again so you can ship the next version of ts-morph without it, if you so desire.

@dsherret
Copy link
Owner

dsherret commented Dec 2, 2023

@ajvincent I'll finish it up right now. I was too busy throughout the week to do it. Hope you feel better soon!

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

Successfully merging a pull request may close this issue.

3 participants