-
Notifications
You must be signed in to change notification settings - Fork 3
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
Porting from Rust SDK to TS SDK #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, well done! Only minor feedback, nothing to hold up the PR.
@@ -0,0 +1,82 @@ | |||
# Hasura NDC Typescript (Deno) Connector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come there are two readmes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is due to deno.land hosting using the /src subdirectory, so it needs to find a readme in there.
See: https://deno.land/x/[email protected]
I'd like to just have one, but not sure how.
* This module exists to be included as a library by the typescript compiler in infer.ts. | ||
* The reason for this is that the user is likely to use the Deno dev tools when developing their functions. | ||
* And they will have Deno in scope. | ||
* This ensures that these references will typecheck correctly in infer.ts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that these references will typecheck correctly in infer.ts.
Hrm, I wonder if this is true. Does Deno itself actually give the any
type to its entire API, or does it actually have typings for all its functions? That could affect the the typechecker results.
For example, any function called using the below typing will return an any
(which will then propagate to anything that uses the return value), but if the Deno tooling actually has real types, then the return type will be whatever it is, and subsequent usages of the return value will typecheck against that type.
Something to investigate later, though, not this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point. I guess if you do some kind of utility types on something Deno returns this could be an issue.
It seems to unify simple uses of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it just seems to work, but actually doesn't. any
is basically disabling the typechecker, so I reckon you could write some code that fails to typecheck in the IDE/normal deno, but passes when run through infer.ts
.
Good enough for now, though. For the purposes of infer.ts
, so long as the user explicitly specifies the function return type, it probably won't affect our inference results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes for sure that's true. But keep in mind this is mainly for deploying already correct code, expecting that the user will have had the Deno tooling already pick up incorrect uses of Deno for them via the LSP. I think for correct Deno use, it shouldn't fail to typecheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes, let's see if we can use the actual Deno interface in the TS compiler in future. I'll put this in the limitations docs for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're already doing 99% of the heavy lifting with Deno, let's see if we can ditch the remainder of the rust and just have a full Deno stack.
Motivation:
connector create
Limitations:
See https://github.com/hasura/ndc-typescript-deno/issues for outstanding issues
Usage has changed slightly. Now the invocation should look as follows for local dev: