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 tuple type narrowing, fix call expression parser #522

Merged
merged 7 commits into from
Oct 10, 2020

Conversation

mooyoul
Copy link
Contributor

@mooyoul mooyoul commented Sep 15, 2020

Changes

Add PropertyAccessExpression support (w/ unit test)

Supports schema generation of below code:

const FOO = { bar: "123"; }
export type MyType = typeof FOO.bar;

Support type Narrowing of tuple type using array-like object condition

Supports schema generation of below code:

type Conditional<T extends any[]> = T extends { length: 2 } ? true : false;
export type TrueType = Conditional<[string, string]>; // true
export type FalseType = Conditional<[string, string, string]>; // false
export type FalseType = Conditional<string[]>; // false
Use TupleType in ArrayLiteralExpression parser by default
Fix broken CallExpressionParser

Resolves issue with 3rd party type imports (e.g. io-ts)

Add integration test with 3rd party io-ts package

Related Issues

@mooyoul
Copy link
Contributor Author

mooyoul commented Sep 15, 2020

Unfortunately, I have no more time to resolve these issues:

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Let's remove the third party tests and boil down the problem to a unit test.

package.json Outdated
@@ -51,6 +55,8 @@
"eslint": "^7.8.1",
"eslint-config-prettier": "^6.11.0",
"eslint-plugin-prettier": "^3.1.4",
"fp-ts": "^2.8.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not rely on specific third-party libraries for the tests and instead boil the test case down to a unit test.

@@ -17,7 +16,7 @@ export class ArrayLiteralExpressionNodeParser implements SubNodeParser {
if (node.elements) {
const elements = node.elements.map((t) => this.childNodeParser.createType(t, context)).filter(notUndefined);

return new ArrayType(new UnionType(elements));
return new TupleType(elements);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -0,0 +1,5 @@
import { assertValidSchema } from "./utils";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this test.

@domoritz domoritz changed the title Minor Improvements Support tuple type narrowing, fix call expression parser Sep 16, 2020
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Let's just fix a few more things and then I will merge the pull request and make a release.

@@ -14,10 +11,19 @@ export class CallExpressionParser implements SubNodeParser {
}
public createType(node: ts.CallExpression, context: Context): BaseType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that covers this code below?

return new TupleType([
new UnionType((type as any).typeArguments[0].types.map((t: any) => new LiteralType(t.value))),
]);
node.arguments.forEach((arg) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use for (const arg of node.arguments) {}

return node.kind === ts.SyntaxKind.PropertyAccessExpression;
}

public createType(node: ts.PropertyAccessExpression, context: Context): BaseType | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that covers this case

@domoritz
Copy link
Member

@mooyoul These changes are great and I would love to merge them. Can you finish up the two small changes?

@domoritz domoritz merged commit 7707141 into vega:master Oct 10, 2020
@domoritz
Copy link
Member

domoritz commented Oct 10, 2020

Hmm, unfortunately, this change breaks

const IDX = {
    foo: 1,
    bar: 1,
};

export const keys = Object.keys as <T>(o: T) => Extract<keyof T, string>[];
export const Foo = keys(IDX);

export type MyType = typeof Foo[number];
Error: Unknown node "IDX" (ts.SyntaxKind = 78)

@domoritz
Copy link
Member

@mooyoul it would be awesome if you could help fix this case above.

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.

Incompatible with io-ts?
2 participants