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 generating typescript interfaces from an abstract sql model #1

Merged
1 commit merged into from
Dec 28, 2020

Conversation

Page-
Copy link
Contributor

@Page- Page- commented Oct 5, 2020

Change-type: minor

@Page- Page- force-pushed the generate-interface branch 4 times, most recently from aea8b15 to 2bd172e Compare October 5, 2020 11:46
Comment on lines +40 to +62
const nullable = f.required ? '' : '?';
return `{ __id: number } | [${referencedInterface}${nullable}]`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik the nullable part should be [] | [T]. It seems that [T?] isn't exactly equivalent since changing the respective line in the SDK will cause the typings to error.
See: https://github.com/balena-io/balena-sdk/blob/b24371c390a65cdfabff2bb41404ed2b4ea1c841/typings/pinejs-client-core.d.ts#L17-L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that might be an issue in the SDK typings @thgreasi and how it does typing magic since [T?] vs [] | [T] should be equivalent and at least on a basic test using

const x: OptionalNavigationResource<Device> = [] as any;
if (Array.isArray(x)) {
	const y = x[0]
}

then y ends up with the same type with either [T?] or [] | [T]

Copy link
Contributor

Choose a reason for hiding this comment

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

Well strictly speaking [T?] means that there is a T or undefined in the single item tuple, while the other describes that it's either a single item T tuple or an empty array.

Copy link
Contributor Author

@Page- Page- Oct 9, 2020

Choose a reason for hiding this comment

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

T[0] will return undefined in both cases @thgreasi and check the following:

let x: [string?]
type l = typeof x.length
// type l = 0 | 1
type e = typeof x[0]
// type e = string | undefined

the typing seems to recognise it's length as 0 | 1 and the element is string | undefined, the same as for [] | [T]:

let x: [] | [string]
type l = typeof x.length
// type l = 0 | 1
type e = typeof x[0]
// type e = string | undefined

test/index.ts Outdated Show resolved Hide resolved
test/index.ts Outdated Show resolved Hide resolved
Comment on lines +53 to +56
case 'ConceptType':
if (opts.mode === 'write') {
return 'number';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a type helper in the SDK which generated the "write" variant given the "read" one.
Imo it's more convenient that having to use two separate types.
See: https://github.com/balena-io/balena-sdk/blob/master/typings/pinejs-client-core.d.ts#L350-L352

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to generate the "write" typings purely from the "read" typings, for instance Date | string is valid for write but the read typing is just string

@Page- Page- force-pushed the generate-interface branch 3 times, most recently from 1ef9735 to 537bd16 Compare December 28, 2020 17:47
@Page- Page- requested review from thgreasi and removed request for CameronDiver December 28, 2020 17:47
@Page- Page- marked this pull request as ready for review December 28, 2020 17:50
@ghost ghost merged commit e370cab into master Dec 28, 2020
@Page- Page- deleted the generate-interface branch December 28, 2020 20:09
This pull request was closed.
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.

2 participants