feat(types): ship automatic TypeScript typings #794

cdaringe · 2018-07-07T22:59:50Z

problem statment

TS typings are out of date
TS typings don't ship by default

partially address #520

solution

have this TS project auto-build its typing artifcacts
- achieve this theoretically via a 3 LOC code change in the tsconfig.json
- address the cascading set of small issues associated with ^

cdaringe · 2018-07-07T23:01:00Z

package.json

@@ -34,6 +34,9 @@
    "prepack": "./scripts/build"
  },
  "dependencies": {
+    "@types/pg": "^7.4.10",
+    "@types/graphql": "^0.8.2",
+    "@types/jsonwebtoken": "<7.2.1",


because our interfaces expose types that use these types, the Typescript documentation states to bundle them as dependencies. i was mixed feelings on this!

cdaringe · 2018-07-07T23:01:19Z

package.json

@@ -75,8 +76,8 @@
    "source-map-support": "^0.4.6",
    "supertest": "^2.0.1",
    "ts-node": "^2.0.0",
-    "tslint": "^4.2.0",
-    "typescript": "2.1.5"
+    "tslint": "^5.10.0",


so we can disable new rules. will discuss later

cdaringe · 2018-07-07T23:01:38Z

scripts/lint

@@ -4,4 +4,4 @@ npm_bin=$(npm bin)

 set -e

-$npm_bin/tslint $(find src -name "*.[tj]s" -a \! -wholename '*/node_modules/*') $@
+$npm_bin/tslint --format verbose $(find src -name "*.[tj]s" -a \! -wholename '*/node_modules/*') $@


adds rule names to output, essentially.

cdaringe · 2018-07-07T23:03:16Z

src/interfaces.ts

+import jwt = require('jsonwebtoken')
+import { GraphQLError } from 'graphql/error'
+
+export namespace Postgraphile {


types that were used in various places i moved into a namespace. for instance, PostGraphileOptions was almost fully duped in 2 places. i defined it once, imported it back two places, and extended it as needed

Capital g please 🤗

cdaringe · 2018-07-07T23:05:10Z

src/postgraphile/cli.ts


 const debugCli = debugFactory('postgraphile:cli')

-// tslint:disable no-console
+// tslint:disable no-console no-shadowed-variable
+// no-shadowed-variable is erroroneously reporting


im betting the older tslint didnt do this :/ it was claiming that plugins in a typedef was shadowed. infact, it was just annotating the type. bonkers AST parse. as you can see, i didn't modify anything to do w/ a variable called plugins in this file.

Nope; this was a legit error (my bad) 5599c82

cdaringe · 2018-07-07T23:05:51Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.d.ts

@@ -1,101 +0,0 @@
-import { IncomingMessage, ServerResponse } from 'http'


all of this content was trashed or moved into interfaces.d.ts, as referenced above

cdaringe · 2018-07-07T23:08:25Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -1,30 +1,49 @@
 /* eslint-disable */// Because we use tslint
+/* tslint:disable:no-any no-var-requires */


and here's the real only significant offender of this PR. to get the build to auto-populate typings, allowJs must be set to false. this was the lone .js in the lib.

this file could use have used some deep, deep tidy before i put my fingers on it. do to the size and complexity of it, rather than put proper types on everything in the true TS spirit, I declared many of the internals as any. this was to prevent any major refactor or create even more change than we already want for this PR.

cdaringe · 2018-07-07T23:09:38Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

+ * - `express`.
+ * - `koa` (2.0).
+ */
+export interface ICreateRequestHandler extends Postgraphile.PostGraphileOptions {


boom! that huge red-file (createPostGraphileHttpRequestHandler.d.ts), gets compacted down to these 9 LOC. mo betta, much betta.

cdaringe · 2018-07-07T23:11:03Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

    if (options.showErrorStack)
      formattedError.stack =
-        options.showErrorStack === 'json'
+        options.showErrorStack


👀 needs discussion. showErrorStack is a bool everywhere else, and the lib wouldn't compile with the above conditional. is this really a supported feature? it looks like in both places, the stack makes it through.

im not sure of the intent.

This was before my time, but I believe there used to be an option where you could have the stack return an array rather than a string. I don't think this is currently possible, but a review of the code would be needed to assert this is the case.

ya. it logically doesn't make sense that showErrorStack would affect the return type of the stack anyway. i'm prone to change this whole logic entirely to:

formattedError.stack = option.showErrorStack ? (error.stack || '') : ''

So it turns out that this is actually a documented feature:

showErrorStack: Enables adding a stack field to the error response. Can be either the boolean true (which results in a single stack string) or the string 'json' (which causes the stack to become an array with elements for each line of the stack). Recommended in development, not recommended in production.
-- https://www.graphile.org/postgraphile/usage-library/#api-postgraphilepgconfig-schemaname-options

in the original ternary, either way, a stack was assigned. this is a bug with the original impl. let's file a bug and address it independently.

i filed #804 for addressing this

cdaringe · 2018-07-07T23:12:35Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

          if (error) {
            return next(error)
          }
          fn(req, res, next)
        })
      }
    },
-    (req, res, next) => next(),
+    (req: any, res: any, next: any) => next(),


honestly, i dont know how this is working at all right now (before i touched it, or after). i can't find next on 245 in scope? our initial value is a function which calls next, which i dont see defined anywhere.

I know how that feels. It’s one of the function arguments: req, res, NEXT 🙃

Basically this is a no-op middleware we use as a base so we can efficiently combine the other middlewares into one middleware. It helps the JIT.

ah, next isnt really used. we may as well have literally used a no-op on 245. 237-241 provides a next implementation for us. doh! thx :)

Yeah this reduce is super confusing; the next is actually called, it's just the inner most "no-op" middleware; each middleware wraps the previous one and we have this be the initial one.

cdaringe · 2018-07-07T23:13:17Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -430,7 +438,7 @@ export default function createPostGraphileHttpRequestHandler(options) {
          req._koaCtx.compress = false
        }
        await new Promise((resolve, reject) => {
-          const stream = sendFile(req, assetPathRelative, {
+          sendFile(req, assetPathRelative, {


ya ya ya. the linter made me do all of this pedantic junk. im sorry!

cdaringe · 2018-07-07T23:13:52Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -742,7 +749,7 @@ export default function createPostGraphileHttpRequestHandler(options) {

      res.setHeader('Content-Type', 'application/json; charset=utf-8')

-      res.end(JSON.stringify(returnArray ? results : results[0]))
+      res.end(JSON.stringify(returnArray ? results : results![0]))


this is a new TS feature i learned recently. assert that <preceding_var> is defined!

This is nice! This has been annoying me for a while 👍

cdaringe · 2018-07-07T23:14:26Z

src/postgraphile/pluginHook.ts

@@ -1,9 +1,10 @@
 /* eslint-disable */// Because we use tslint


only change below is changing where types are imported from

cdaringe · 2018-07-07T23:14:54Z

src/postgraphile/postgraphile.ts

@@ -1,188 +1,18 @@
 import { Pool, PoolConfig } from 'pg'


only changes below are changing where types are imported from, and migrating these types to a common interfaces file

cdaringe · 2018-07-07T23:15:34Z

tsconfig.json

@@ -14,10 +16,9 @@
    "suppressImplicitAnyIndexErrors": true,
    "strictNullChecks": true,
    "noFallthroughCasesInSwitch": true,
-    "noUnusedParameters": true,
+    "noUnusedParameters": false,


because so often we have (req, res, next) where res isn't used

Can we configure it like ESLint using these rules rather than disabling it outright? I'm not sure how flexible it is in this regard.

we don't care if parameters start with underscores (e.g. _res would be ignored)

we don't care if one of the parameters after this one is used (e.g. (req, res, next) => next() shouldn't raise)

ah, perfect. i wasn't aware that that was a thing. looks like ts honors it as well. 2396b0e

cdaringe · 2018-07-07T23:15:43Z

tsconfig.json

@@ -1,6 +1,8 @@
 {
  "compilerOptions": {
-    "allowJs": true,
+    "declaration": true,


cdaringe · 2018-07-07T23:15:57Z

typings/mixed.d.ts

@@ -1,18 +0,0 @@
-/**


👋 bye bye

cdaringe · 2018-07-07T23:16:07Z

typings/object.d.ts

@@ -1,4 +0,0 @@
-/**


👋 adddddiiioos!

cdaringe · 2018-07-07T23:17:13Z

a net -1K LOC! i say that's a win :)

benjie · 2018-07-08T08:03:28Z

src/interfaces.ts

+import jwt = require('jsonwebtoken')
+import { GraphQLError } from 'graphql/error'
+
+export namespace Postgraphile {


Capital g please 🤗

benjie · 2018-07-08T08:31:22Z

src/interfaces.ts

+    (ctx: { req: IncomingMessage, res: ServerResponse }, next: () => void): Promise<void>
+    getGraphQLSchema: () => Promise<GraphQLSchema>
+    formatError: (e: GraphQLError) => GraphQLError
+    pgPool: Pool


I think this has more stuff on it now, I should check.

withPostGraphileContextFromReqRes should be added:

https://github.com/graphile/postgraphile/blob/master/src/postgraphile/http/createPostGraphileHttpRequestHandler.js#L813-L816

20a50fe

this one was a bit of a drag, inducing a little more typing refactor than I had wanted. ill see if i can annotate it. i bundled your remark from https://github.com/graphile/postgraphile/pull/794/files#r200834248 in this commit too.

benjie · 2018-07-08T08:46:12Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

          if (error) {
            return next(error)
          }
          fn(req, res, next)
        })
      }
    },
-    (req, res, next) => next(),
+    (req: any, res: any, next: any) => next(),


I know how that feels. It’s one of the function arguments: req, res, NEXT 🙃

Basically this is a no-op middleware we use as a base so we can efficiently combine the other middlewares into one middleware. It helps the JIT.

benjie

Excellent start! Here's some feedback and although it took me a solid 45 minutes to go through this I've not reviewed it in enough detail to be confident I've not missed anything.

Please make sure any changes from now are performed as additional commits (please do not rebase!) to make further reviews easier.

benjie · 2018-07-08T08:57:50Z

src/interfaces.ts

+    (ctx: { req: IncomingMessage, res: ServerResponse }, next: () => void): Promise<void>
+    getGraphQLSchema: () => Promise<GraphQLSchema>
+    formatError: (e: GraphQLError) => GraphQLError
+    pgPool: Pool


withPostGraphileContextFromReqRes should be added:

https://github.com/graphile/postgraphile/blob/master/src/postgraphile/http/createPostGraphileHttpRequestHandler.js#L813-L816

benjie · 2018-07-08T09:00:15Z

src/postgraphile/http/__tests__/createPostGraphileHttpRequestHandler-test.js

-      ).toBeInstanceOf(http.ServerResponse)
-    })
-  })
-}


I don't think you've replaced this test file after deleting it?

(This is possibly where 855 of the removed 1K LOC came from 😳 )

oh brother. 🤦‍♂️

0ab968a

benjie · 2018-07-08T09:01:40Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -36,12 +55,12 @@ const sendFile = require('send')
 const LRU = require('lru-cache')
 const crypto = require('crypto')

-const calculateQueryHash = queryString => crypto.createHash('sha1').update(queryString).digest('base64')
+const calculateQueryHash = (query: string) => crypto.createHash('sha1').update(query).digest('base64')


I prefer queryString: string since "query string" is what I would call it aloud. "query" has a different meaning to me.

benjie · 2018-07-08T09:07:52Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -94,15 +111,14 @@ const origGraphiqlHtml = new Promise((resolve, reject) => {
 * We need to be able to share the withPostGraphileContext logic between HTTP
 * and websockets
 */
-const withPostGraphileContextFromReqResGenerator = options => {
+function withPostGraphileContextFromReqResGenerator(options: ICreateRequestHandler): any {


Rather than any, consider using something like:

function withPostGraphileContextFromReqResGenerator<T>(options: ICreateRequestHandler): (req: IncomingMessage, res: ServerResponse, moreOptions: any, fn: (pgClient: PoolClient) => T) => T

You can use any instead of T if you like. Effectively whatever is returned by the callback function fn will be returned by the function returned from withPostGraphileContextFromReqResGenerator

benjie · 2018-07-08T09:14:18Z

src/interfaces.ts

+    // "omit" (default) - relay connections only,
+    // "only" - simple collections only (no Relay connections),
+    // "both" - both
+    simpleCollections?: 'omit' | 'both' | 'only',


We should consider adding [propName: string]: any here because PostGraphile plugins can add additional options to this object (e.g. the supporter plugin adds simpleSubscriptions as an option)

benjie · 2018-07-08T09:22:55Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -850,6 +857,13 @@ function addCORSHeaders(res) {
  )
 }

+function createBadAuthorizationHeaderError(): void {
+  throw httpError(


Don't throw here, we throw down below (and the name is just create)

whoops! great catch. thx. fb7430b

benjie · 2018-07-08T09:26:42Z

src/postgraphile/http/setupServerSentEvents.ts

-  const sse = str => {
-    if (isKoa) {
+  const sse = (str: string) => {
+    if (isKoa && stream) {


Ugh. This is one of the things I hate about typed JS. The logic here would be confusing to the reader as they'd be "if it's koa but there is no stream then do res.write? Why would there not be a stream...?" and then read the code to find it's impossible to not have a stream if it's Koa.

Can you use stream!.write(str) below instead so that it's easier to read and doesn't change the compiled code?

benjie · 2018-07-08T09:29:34Z

tsconfig.json

@@ -14,10 +16,9 @@
    "suppressImplicitAnyIndexErrors": true,
    "strictNullChecks": true,
    "noFallthroughCasesInSwitch": true,
-    "noUnusedParameters": true,
+    "noUnusedParameters": false,


Can we configure it like ESLint using these rules rather than disabling it outright? I'm not sure how flexible it is in this regard.

we don't care if parameters start with underscores (e.g. _res would be ignored)

we don't care if one of the parameters after this one is used (e.g. (req, res, next) => next() shouldn't raise)

benjie · 2018-07-08T09:31:17Z

tsconfig.json

    "noUnusedLocals": true
  },
-  "include": ["src", "typings"],


Why have you removed this? I store a lot of extra stuff (tests for user issues, WIP code, local test scripts, etc) that I wouldn't want to be affected by tsconfig

mmm. i think i see what you mean. flipping tsconfig to allowJs: false prevents .js files from being copied into the build, so those files would be omitted anyway if we auto gen the declarations. if that content needs to ship, we can do it in a post processing step. does that work? are you certain those need to be shipped?

benjie · 2018-07-08T09:32:29Z

typings/pg-sql2.d.ts

@@ -6,6 +6,6 @@ declare module 'pg-sql2' {

  export function compile(query: SQLQuery): QueryConfig
  export function query(strings: TemplateStringsArray, ...values: Array<SQLNode | SQLQuery>): SQLQuery
-  export function value(val: mixed): SQLNode
+  export function value(val: any): SQLNode


Why was this change required? Specifically this should only receive JSON-like values, I think...

i confess. laziness as i wasn't not sure how to import a locally defined interface :/ ../src/interfaces

benjie · 2018-07-20T17:26:22Z

Hour and a half into reviewing this, few minor changes but looking good so far. It was too intense to review on it's own so I've checked out the src directory as of master and then re-implemented the changes myself over the top. As my diff gets smaller and smaller we get closer to approving this PR 👍

All the minor files are now done; just the major one createPostGraphileHttpRequestHandler and new companion koaMiddleware to go. 😅

cdaringe · 2018-07-20T18:38:05Z

Cool! Ya, sorry it blew up. allowJs being false really took this from an easy win to beast.

benjie · 2018-07-20T18:43:51Z

😅 Okay; I've reviewed it fully and made a load of changes and I'm pretty much ready to merge it. I need to give it a last check over (and fix any CI errors) but I'm already running super late for my evening plans so I'm going to head out.

I've reverted a few things to their previous behaviour.

benjie · 2018-07-21T09:09:44Z

I just went through the compiled bundle and inspected every d.ts file; the external requires that we need to bundle types for are:

    "@types/graphql": "^0.8.2",
    "@types/jsonwebtoken": "<7.2.1",
    "@types/koa": "2.0.44",
    "@types/pg": "^7.4.10",

And postgraphile-core which doesn't currently have types. We have a declaration in typings/postgraphile-core.d.ts but I don't think that's being referenced by the build... How do we solve this @cdaringe?

cdaringe · 2018-07-21T18:11:37Z

but I don't think that's being referenced by the build

it's being referenced by the build, but not being included in it. based on your remark in #520, i think you already figured it out :)

thanks for taking to the time to get back over all of this!

benjie · 2018-07-21T18:41:48Z

Thank you for doing the work! Being TypeScript through and through should make a big difference to a lot of users, and greater confidence in PRs.

feat(typings): ship automatic ts typings by default

74d5a2e

cdaringe commented Jul 7, 2018

View reviewed changes

tsconfig.json

@@ -1,6 +1,8 @@

{

"compilerOptions": {

"allowJs": true,

"declaration": true,

Copy link

Contributor Author

cdaringe Jul 7, 2018

Choose a reason for hiding this comment

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

🎉

cdaringe commented Jul 7, 2018

View reviewed changes

typings/mixed.d.ts

@@ -1,18 +0,0 @@

/**

Copy link

Contributor Author

cdaringe Jul 7, 2018

Choose a reason for hiding this comment

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

👋 bye bye

cdaringe commented Jul 7, 2018

View reviewed changes

typings/object.d.ts

@@ -1,4 +0,0 @@

/**

Copy link

Contributor Author

cdaringe Jul 7, 2018

Choose a reason for hiding this comment

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

👋 adddddiiioos!

cdaringe mentioned this pull request Jul 7, 2018

How to import postgraphql in a TypeScript project? #520

Closed

cdaringe changed the title ~~wip: feat(typings): ship automatic ts typings by default~~ feat(typings): ship automatic ts typings by default Jul 8, 2018

benjie reviewed Jul 8, 2018

View reviewed changes

benjie requested changes Jul 8, 2018

View reviewed changes

cdaringe added 6 commits July 8, 2018 09:31

refactor(namespace): Postgraphile => PostGraphile

a5b736f

feat(PostGraphileOptions): allow arbitrary keys for plugin support

af448cc

refactor(middleware): clarify middleware types and flow

20a50fe

fix(middleware): restore koa interface

8b8c490

chore(createHandler): add GQL schema type

82e9d00

chore(createHandler): add results type

aa99779

benjie added 5 commits July 20, 2018 17:28

Remove shadowed variable

5599c82

Always run Koa tests

282404d

Tweaks

16135d4

Small tweaks

ce129b7

SSE types

7025ec5

benjie added 8 commits July 20, 2018 18:54

Stronger typing for Koa middleware

2479fec

Koa types

5d76cfc

const to import

cf1207a

Stronger types

0c22389

Stronger types

3bed5ed

Stronger types

3fa5fe0

Stronger types

550ea53

Stronger types

cbbfca6

benjie added 2 commits July 20, 2018 19:40

Restore (a, b, c) non-async middleware

a81ef96

Fix callback types

e6e1176

benjie added 2 commits July 21, 2018 09:57

Satisfy TypeScript

bf59d1f

Move types from dependencies to devDependencies

fafa7ab

benjie added 3 commits July 21, 2018 10:14

Add types declaration to package.json

ad33fc0

I hate semicolon-less JS/TS soooooo much

d1c5758

Move queryCacheMaxSize to documented section

54aa4b4

benjie changed the title ~~feat(typings): ship automatic ts typings by default~~ feat(types): ship automatic TypeScript typings Jul 21, 2018

benjie merged commit 89b0900 into graphile:master Jul 21, 2018

cdaringe deleted the fix/typings branch July 21, 2018 18:07

benjie mentioned this pull request Jul 23, 2018

chore(lint): use more standard TSLint config #810

Merged

		@@ -1,101 +0,0 @@
		import { IncomingMessage, ServerResponse } from 'http'

		@@ -1,30 +1,49 @@
		/* eslint-disable */// Because we use tslint
		/* tslint:disable:no-any no-var-requires */

		@@ -1,9 +1,10 @@
		/* eslint-disable */// Because we use tslint

feat(types): ship automatic TypeScript typings #794

feat(types): ship automatic TypeScript typings #794

Conversation

cdaringe commented Jul 7, 2018 • edited Loading

problem statment

solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdaringe Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdaringe Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjie Jul 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjie Jul 8, 2018 • edited Loading

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

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

cdaringe commented Jul 7, 2018

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

benjie Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

benjie 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

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

benjie commented Jul 20, 2018

cdaringe commented Jul 20, 2018

benjie commented Jul 20, 2018

benjie commented Jul 21, 2018

cdaringe commented Jul 21, 2018

benjie commented Jul 21, 2018

cdaringe commented Jul 7, 2018 •

edited

Loading

cdaringe Jul 7, 2018 •

edited

Loading

cdaringe Jul 8, 2018 •

edited

Loading

benjie Jul 20, 2018 •

edited

Loading

benjie Jul 8, 2018 •

edited

Loading

benjie Jul 8, 2018 •

edited

Loading