Add basic generics support #37

heyitsaamir · 2020-12-01T20:27:31Z

This PR adds basic generics support to ts2gql.

Example

export interface MultipleGenerics<T, U> {
  objectT: T;
  arrayObjectU: U[];
  extraNonGenericProp: boolean;
}

export interface StringObjectNumberArray extends MultipleGenerics<string, number> {
  ownProp: boolean;
}

export interface StringObjectNumberArrayObjectNumberArray extends MultipleGenerics<StringObjectNumberArray, number> {
  ownProp: boolean;
}

results in:

type StringObjectNumberArray {
  arrayObjectU: [Float]
  extraNonGenericProp: Boolean
  objectT: String
  ownProp: Boolean
}

type StringObjectNumberArrayObjectNumberArray {
  arrayObjectU: [Float]
  extraNonGenericProp: Boolean
  objectT: StringObjectNumberArray
  ownProp: Boolean
}

Left to do:

Tests for cases that will throw
Adding a few more tests for multiple properties for the same type

nevir

High level review, structurally I think it looks pretty good. As to whether we think it's a good idea to support, I leave that to others ;)

Only niggle on the terminology side is that it's really "templated type" / "type templates" (affecting only the type system) rather than "generics" (tends to mean generated code per type in most contexts)

nevir · 2020-12-01T22:07:41Z

.vscode/launch.json

+      "port": 9229
+    }
+  ]
+}


Awesome, thanks for setting this up!

nevir · 2020-12-01T22:14:26Z

src/Collector.ts

+        for (const clause of node.typeParameters) {
+          const type = this.checker.getTypeAtLocation(clause);
+          const symbol = type.getSymbol();
+          if (symbol) {


Hmm, in what case would you not have a symbol here? (seems like it might be worth an assertion that the symbol exists?)

nevir · 2020-12-01T22:14:52Z

src/Collector.ts

+            if (typeRef.type === 'expressionWithTypeNode' && typeRef.referenceNodes.some(r => r.type === 'reference' && r.referenceNodes.length > 0)) {
+              console.error(type);
+              console.error(type.getSourceFile().fileName);
+              throw new Error(`Generic inheritance cannot have another generic. Try using a named type instead. ${node.getText()}`);


No nested generics? booo! :P

nevir · 2020-12-01T22:15:11Z

src/Collector.ts

+              console.error(type.getSourceFile().fileName);
+              throw new Error(`Generic inheritance cannot have another generic. Try using a named type instead. ${node.getText()}`);
+            }
+            // get the symbol + typeref. if it's


Hmm, you're leaving me hanging here

nevir · 2020-12-01T22:18:50Z

src/Collector.ts

    }

    return this._addType(node, () => {
      const inherits = [];
+      const typeParameters: string[] = [];
      if (node.heritageClauses) {


For future PRs, you might also want to support type aliases. E.g.

type StringNumberArray = MultipleGenerics<string, number>;

(if we are likely to do this)

Could add a TODO comment for this if you plan to punt for now, @heyitsaamir

nevir · 2020-12-01T22:23:59Z

src/Emitter.ts

+                if (m.type === 'property') {
+                  if (m.signature.type === 'genericPropertyNode' && m.signature.signature === t) {
+                    newGenericMapForNode.set(m.name, referenceNode)
+                  } else if (m.signature.type === 'array') {
+                    const arrayType = m.signature.elements[0];
+                    if (arrayType.type === 'genericPropertyNode' && arrayType.signature === t) {
+                      newGenericMapForNode.set(m.name, referenceNode)
+                    }
+                  }
+                }


You may end up wanting to make this its own (recursive) visitor in order to handle more complex types if we see them in use (e.g. things like foo: { bar: TSomeTemplateType }

nevir · 2020-12-01T22:24:40Z

src/Emitter.ts

+                  if (m.signature.type === 'genericPropertyNode' && m.signature.signature === t) {
+                    newGenericMapForNode.set(m.name, referenceNode)
+                  } else if (m.signature.type === 'array') {
+                    const arrayType = m.signature.elements[0];


Definitely will want to handle other elements (unions, I think) or assert this is the only case here

finnigantime

Awesome! LGTM so far.

finnigantime · 2020-12-04T16:11:37Z

scripts/compile-watch.sh

+source ./scripts/include/node.sh
+
+npm run clean
+tsc-watch "${@}"


finnigantime · 2020-12-04T16:14:47Z

src/Collector.ts

    }

    return this._addType(node, () => {
      const inherits = [];
+      const typeParameters: string[] = [];
      if (node.heritageClauses) {


Could add a TODO comment for this if you plan to punt for now, @heyitsaamir

finnigantime · 2020-12-04T16:17:45Z

src/Collector.ts

-  _referenceForSymbol(symbol:typescript.Symbol):types.ReferenceNode {
-    this._walkSymbol(symbol);
+  _referenceForSymbol(symbol: typescript.Symbol, node: typescript.TypeReferenceNode): types.ReferenceNode {
+    this._walkSymbol(symbol); //getTypeReferenceName


finnigantime · 2020-12-04T16:19:18Z

src/Emitter.ts

    return `enum ${this._name(name)} {\n${this._indent(node.values)}\n}`;
  }

-  _emitExpression = (node:Types.Node):string => {
+  _emitExpression = (node: Types.Node, possibleGenericReferenceNode?: Types.ReferenceNode): string => {


why have the possible prefix? The ? already indicates the param may be undefined

finnigantime · 2020-12-04T16:19:57Z

src/Emitter.ts

        })
        .join(', ');
+    } else if (node.type === 'genericPropertyNode') {
+      if (!possibleGenericReferenceNode) {
+        throw new Error(`node ${node.signature} was not resolved`);


In what case is this expected? This error message doesn't point at the cause or resolution.

finnigantime · 2020-12-04T16:20:24Z

src/Emitter.ts

@@ -219,7 +234,7 @@ export default class Emitter {
        if (interfaceNode.inherits.length > 1) {
          throw new Error(`No support for multiple inheritence: ${JSON.stringify(interfaceNode.inherits)}`);
        } else if (interfaceNode.inherits.length === 1) {
-          const supertype:Types.Node = this.types[interfaceNode.inherits[0]];
+          const supertype: Types.Node = this.types[interfaceNode.inherits[0].name];


was this an unrelated bug?

finnigantime · 2020-12-04T16:21:01Z

src/Emitter.ts

+        }
+
+      } else {
+        // throw


ajawaid added 6 commits November 22, 2020 15:40

Get basic generics working

ed1e206

Add unit tests

ded19ba

Fix

7ba66a0

Remove junit

4201586

Remove extra comments

262f8d3

Fix comment

de99ab7

heyitsaamir requested review from finnigantime and nevir December 1, 2020 20:38

nevir reviewed Dec 1, 2020

View reviewed changes

finnigantime reviewed Dec 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic generics support #37

Add basic generics support #37

heyitsaamir commented Dec 1, 2020 •

edited

Loading

nevir left a comment

nevir Dec 1, 2020

nevir Dec 1, 2020

nevir Dec 1, 2020

nevir Dec 1, 2020

nevir Dec 1, 2020

finnigantime Dec 4, 2020

nevir Dec 1, 2020

nevir Dec 1, 2020

finnigantime left a comment

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

finnigantime Dec 4, 2020

Add basic generics support #37

Are you sure you want to change the base?

Add basic generics support #37

Conversation

heyitsaamir commented Dec 1, 2020 • edited Loading

Example

Left to do:

nevir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finnigantime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyitsaamir commented Dec 1, 2020 •

edited

Loading