-
-
Notifications
You must be signed in to change notification settings - Fork 569
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
Typescript definition generation added to tsconfig.json #383
Conversation
@@ -1,6 +1,5 @@ | |||
{ | |||
"compilerOptions": { | |||
"allowJs": true, |
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.
Why did you remove allowJs
? Some of PostGraphQL’s files like createPostGraphQLHttpRequestHandler.js
are written in plain JavaScript as it is more convenient, and we naturally play a little more loosely with types.
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.
The problem is you can not use declaration: true
with allowJS: true
thats why I removed it form the configuration.
Compiling with TypeScript
error TS5053: Option 'allowJs' cannot be specified with option 'declaration'.
Thanks for the hint I checked the result the http
folder is missing in the build. There are already open issues for solving this problem: microsoft/TypeScript#7546
As alternative would you be ok to switch completely to TS? Cause for the file you described there is already an external .d.ts
file. Wouldn't it much easier to maintain when both files would be combined? Beside that there are only 3 other JS files left (except the tests).
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.
The reason I avoided going full TypeScript is that we would have to install a lot of typings packages that would only be used in that one file 😣
If it’s not too hard, and we don’t have to add too many packages, I’d be fine with going for it. Another thing to note is that a lot of tests are written in plain JS and they would be very hard to convert. I think you wouldn’t need to convert them because they are executed by Jest. You may just need to add a custom transformer for Jest and test .js
files.
Another alternative is we could just provide declarations for our exports, which while duplicative is a valid approach (their equivalence can be tested). Or do you want declarations for internal modules as well?
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.
With the recent typescript version there is no need at all to define declaration files for packages. There are totally optional now. See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-1.html and the "Untyped Imports" section.
I don't think that the tests needs to be ported to typescript cause there irrelevant for the production build. Maybe the way to go would be two different tsconfig.json
files one for testing and one for building where we enable/disable what is needed?
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.
We do turn on noImplicitAny
, though 😣
Yeah. The tests really shouldn’t be ported. I don’t think we’ll need two tsconfig.json
files, but if you can get us running with two we can see about shuffling them around 👍
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 will give it a try.
tsconfig.json
Outdated
@@ -16,7 +15,8 @@ | |||
"strictNullChecks": true, | |||
"noFallthroughCasesInSwitch": true, | |||
"noUnusedParameters": true, | |||
"noUnusedLocals": true | |||
"noUnusedLocals": true, | |||
"declaration": true |
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.
Nit: could you put this bellow line 4? I like to put all of the file output options together 😊
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.
Sure :)
Nice! Just a couple of comments, but we’d be happy to share the types with everyone 😊 |
05a18e8
to
ae88676
Compare
You already write postgraphql in typescript with this pull request all other typescript progammers can benefit from this when they use postgraphql as library instead as cli tool.
ae88676
to
955c9e8
Compare
@calebmer Is this good to merge? |
+1 for making PostGraphQL directly importable into TypeScript projects. I'm using a typings workaround for it now in my project, but would love not to have to do that. |
@benjie @calebmer any updates on this? we're using postgraphql in our project, and we have a sizable dev team. This pull request would be excellent, instead of having to npm install directly from github. If it could also be merged into @types that would be great. I have sufficient time to do this, but it seems done already. Could y'all please merge if it's acceptable? Cheers |
Hi @murpyslaw, v4 is moving over to using Flow (slowly - currently all the graphile-build tools that generate the schema are in Flow but the PostGraphQL wrapper is still TypeScript) so I fear merging TypeScript support now only to have it deprecated very shortly thereafter would be annoying. I'd rather export explicit hand-written library typings than auto-generated ones from the code so that we can easily move between type systems without breaking people's code. If you wanted to craft these you'd be most welcome to. (Incidentally are you using v3 or v4?) Cheers, Benjie. |
I see. I was not aware of this change. I've never worked with Flow before, nor am I familiar with whether or not it can output type definition files or if it just checks against the JS code. Incidentally, our whole stack is in TS. Could you comment on how this architectural change could possibly affect our project, and why postgraphql has chosen to move away from TS? I am wondering whether, at this early stage of our project, this might be a good move to follow. Postgraphql is an integral part of it, one which we've grown very fond of. I'd really dislike having to rewrite resolvers in TS or POJO, and move all of our business logic out of the DB, which is something which we've recently committed to. Thanks for your time, P.S. We are on |
My $0.02: I'm agnostic to using Flow or TypeScript... they end up being quite similar in structure anyway, and the nice thing about Flow is that you can upgrade your own codebase to being typed gradually, rather than needing to type the whole thing immediately (if working with a legacy codebase). I would actually support the move to Flow... our devs are using it with React and React Native and enjoying it... wouldn't be the worst to have a standard toolset across the whole codebase 😄 @murpyslaw in the meantime, I'm not sure how much typing you need, but if you just need types for the middleware, please consider using my simple workaround for the postgraphql middleware: https://gist.github.com/zopf/36770cbe73ecb2617c5fa7da99fc8ed6 -- just put |
If someone wants to maintain a typings package for PostGraphQL that would be very welcome 👍
Regarding how it affects your app, so long as you just use the public interfaces I don't think it should. Honestly I'd just treat all packages on npm as "JavaScript" and add typings for whatever language you choose to use at the application level (e.g. by pulling them in from typings repos or writing them myself). So long as our code runs on Node.js and exposes the same interface I don't see any reason that we shouldn't be able to move to BuckleScript or Elm or any other language. And the great thing about implementing independent typings files is that we can have typings for all the different languages and if we later change to a different language the typings should still be valid (so long as we expose the same interface).
As for why:
Happy to hear that!
I use PostGraphQL from vanilla JS projects just fine; I don't see any reason you'd need to move away. If you were to move away, there'd be no need to move your business logic out of the DB - just call that same logic from your new implementation! That's one of the great things about PostGraphQL - there's no lock-in because all the important parts will remain - we're just the thing that exposes that. I've used this to enable microservices to talk directly to the DB before and have the same access constraints as PostGraphQL would have. |
@zopf Thanks for the link, unfortunately these are not all the types that this MR provides, hence the thread here. Yes we do use the public interfaces, it's just that they're not exposed. I suppose I rely so heavily on this package, that I could volunteer to do the DefinitelyTyped for it, which will automatically pull it in to @calebmer @benjie is this mergeable into Also can't dts-gen be run over the codebase to produce the required .d.ts files? All that would need to happen is this MR goes through and exports all the relevant types, the rest can be automated, without any human interaction. Cheers |
So reading through Caleb's comments from before; the issue preventing this from being merged seems to be the removal of I'd definitely support you doing the DefinitelyTyped for it. I have no experience with dts-gen. |
This is a really exciting project. Thanks to @benjie for all his work on this. I get that maintaining typings is a lot of work, but having them available via |
This is about adding typings directly to the library. I'd definitely support someone writing/maintaining typings via DefinitelyTyped. You can send PRs to them directly, I think 👍 |
You already write postgraphql in typescript with this pull request all other typescript programmers (like me) can benefit from this when they use postgraphql as library instead as cli tool.
I just enabled
declaration
in thetsconfig.json
and added some needed exports to get the.d.ts
generation working.