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

How to import postgraphql in a TypeScript project? #520

Closed
zopf opened this issue Jul 19, 2017 · 35 comments
Closed

How to import postgraphql in a TypeScript project? #520

zopf opened this issue Jul 19, 2017 · 35 comments

Comments

@zopf
Copy link
Contributor

zopf commented Jul 19, 2017

I am fairly new to TypeScript, so forgive me if I'm making a newbie mistake.

I am attempting to import the postgraphql module in my index.ts file, like so:

import * as postgraphql from 'postgraphql';

TypeScript is telling me that Module ''postgraphql'' resolves to a non-module entity and cannot be imported using this construct.

For my other imports, I installed their typings using e.g. npm install --save-dev @types/underscore, but it does not appear there is a @types/postgraphql module. These typings come from https://github.com/DefinitelyTyped/DefinitelyTyped.

How do I go about setting up the proper typings for the postgraphql module so that I can use it in a TypeScript project?

@ar4mirez
Copy link

Did you tried just doing:

import postgraphql from 'postgraphql'

You would do import * as postgraphql from 'postgraphql'; if postgresql exports multiple keys.

Hope this helps. 🤞

@zopf
Copy link
Contributor Author

zopf commented Jul 19, 2017

I did indeed try that, but then it's an even simpler error: Cannot find module 'postgraphql'.

That is the same type of error that appears when I have not installed the typings for a module using e.g. npm install --save-dev @types/underscore.

@benjie
Copy link
Member

benjie commented Jul 19, 2017

Try

import postgraphql = require('postgraphql')

You can also add typings yourself; e.g. here's some I had to make for v4:

https://github.com/postgraphql/postgraphql/blob/1417bc37b62bae165564eaf8e844c32b312a4c4e/typings/postgraphql-build.d.ts

We're probably going to be moving from TypeScript to Flow at some point, FYI.

@zopf
Copy link
Contributor Author

zopf commented Jul 19, 2017

Still get the same Cannot find module 'postgraphql'. error with the require statement, plus another error because I have my tsconfig.module set to es2015;)

I'm making my own typings now.

Based upon this spec ( http://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html ) it seems like we should probably either have a types field in the package.json or submit a @types npm module, but the types field or default index.d.ts is preferred. I'll make a PR...

@zopf
Copy link
Contributor Author

zopf commented Jul 19, 2017

Ok hm I'm not going to make a PR just yet because, well, I don't know enough about postgraphql's build process and structure to put the definitions in the right place. My guess is that it should go in index.d.ts at the root, and the build script / npm deploy script should be updated to include it in the npm package, but please let me know where it should actually go.

Here is the type definition that I'm testing out in my project now, which seems to compile properly:

declare module 'postgraphql' {

  import { Pool, PoolConfig } from 'pg';
  import { IncomingMessage, ServerResponse } from 'http'

  export type mixed = {} | string | number | boolean | undefined | null

  export interface HttpRequestHandler {
    (req: IncomingMessage, res: ServerResponse, next?: (error?: mixed) => void): void
    (ctx: { req: IncomingMessage, res: ServerResponse }, next: () => void): Promise<void>
  }

  export type PostGraphQLOptions = {
    classicIds?: boolean,
    dynamicJson?: boolean,
    graphqlRoute?: string,
    graphiqlRoute?: string,
    graphiql?: boolean,
    pgDefaultRole?: string,
    jwtSecret?: string,
    jwtAudiences?: Array<string>,
    jwtRole?: Array<string>,
    jwtPgTypeIdentifier?: string,
    watchPg?: boolean,
    showErrorStack?: boolean,
    disableQueryLog?: boolean,
    disableDefaultMutations?: boolean,
    enableCors?: boolean,
    exportJsonSchemaPath?: string,
    exportGqlSchemaPath?: string,
    bodySizeLimit?: string,
    pgSettings?: { [key: string]: mixed },
  }

  export function postgraphql (poolOrConfig?: Pool | PoolConfig | string, schema?: string | Array<string>, options?: PostGraphQLOptions): HttpRequestHandler
  export function postgraphql (poolOrConfig?: Pool | PoolConfig | string, options?: PostGraphQLOptions): HttpRequestHandler
}

@benjie
Copy link
Member

benjie commented Jul 19, 2017

That looks about right. There's work in #383 to add typings to the compiled version but I've not had time to read up enough on it to be sure, this is the only TypeScript project I work on.

@mlegenhausen
Copy link

@benjie are you still planing to change to Flow? Maybe I can take again a look my ticket cause typescript has evolved a lot during my absence.

@benjie
Copy link
Member

benjie commented Feb 21, 2018

Honestly, both TypeScript and Flow cause quite significant maintenance issues - given I only work on PostGraphile a few days a month keeping either of them ticking over is a PITA. I'm considering removing them entirely. I've lost a good 5 hours of PostGraphile development time to TypeScript in the last 2 months due to library updates breaking types and thus breaking CI; and I'm currently (literally right now) having to fix a load of Flow stuff in graphile-build because I upgraded GraphQL which brings new types and suddenly I've got 269 errors to solve.

For a project you're working on full time they are both great tools, but I sink too high a percentage of my PostGraphile time into solving issues with them. Both tools in both projects are out of date because updating them is too time consuming. In addition graphile-build being plugin based does not lend itself to strong typing - particularly it's Build object is highly dynamic.

@mlegenhausen
Copy link

Honestly, both TypeScript and Flow cause quite significant maintenance issues

I can confirm this is annoying.

given I only work on PostGraphile a few days a month keeping either of them ticking over is a PITA.

From my experience it will only get more worse without types. Especially when you only work part time on it. How do you remember which type which variable is? Especially in such a big system. These errors are normally something good. Without them it would be try and error and you need to hope that your unit tests will catch them. This does of cause only apply when the types are also applied correctly.

I would also argue that it is easier to contribute to a good typed system that to something where you guess the whole time which variable is which type?

Maybe I can find the time and take a look in upgrade typescript, so I can better understand where the problems are.

@benjie
Copy link
Member

benjie commented Feb 21, 2018

I'm not too worried about myself forgetting the types; however there is defensiveness in that it'll protect from others breaking things in PRs, like linting and tests do 👍

Sending a PR to a typed project is good if you're adept at that tool however, many people are not - so having things in TypeScript or Flow can be a barrier to contributions over vanilla JS. So you generally have a smaller pool of possibly higher quality PRs.

The amount of time spent just keeping these tools ticking over when you just want to update an unrelated dependency in package.json is not to be underestimated. You can probably tell I'm a bit annoyed about it today, so probably not in the right frame of mind to make any concrete decisions (the temptation to just throw all my toys out the pram (remove types from everything and stick with vanilla JS) has been intense today...). If you are able to upgrade PostGraphile to the latest TypeScript that would be brilliant - you can do it directly on the postgraphile branch which isn't too large these days.

@cdaringe
Copy link
Contributor

not that opinions are being solicited, here's an opinion from a random guy on the web 🌎 .

i've started with and injected TS into many projects over the past two years, only to gut it soon after. it's been a constant annoyance with the same emotion you've expressed above.

my latest effort however, has been much more fruitful & painless. my perception is that the community has grown, typings are updated much faster now, and there are provisions now to extend incomplete/broken interfaces so as to unblock users.

long-term TS-hater here, now siding on slightly TS positive opinion 👍

TS editor support is >> than flow too, which is something to consider!

@devuxer
Copy link

devuxer commented May 2, 2018

Before the discussion here turned to TypeScript vs. Flow, it looks as if @zopf got pretty close to making a PR for his d.ts file. This seems to have stalled because this request is still pending an answer:

My guess is that it should go in index.d.ts at the root, and the build script / npm deploy script should be updated to include it in the npm package, but please let me know where it should actually go.

I don't currently have much knowledge of how DefinitelyTyped works, but I'm curious how close @zopf's proposed d.ts file is to the current API. (Clearly, the name of the project needs to be updated from postgraphql to postgraphile, but is everything else up to date?)

And once the d.ts is brought up to date, how much effort would it take to publish it to DefinitelyTyped?

@devuxer
Copy link

devuxer commented May 2, 2018

@benjie, Regarding the options, given that the source file is already in TypeScript, that should be pretty straightforward.

While thinking about typings, it might make sense to update the documentation as well. The use as library page is up to date except for the absence of handleErrors.

I'm not quite sure what to do about the backwards compatibility issue. I'll have to read up on that more.

@devuxer
Copy link

devuxer commented May 5, 2018

@benjie,

Made some progress this morning on the typings:

  • Created an updated typings file for postgraphile based on your input above.
  • Since postgraphile has dependencies on postgraphile-core, created a typings file for postgraphile-core, but only attempted to create typings needed by postgraphile (my assumption was that, if postgraphile doesn't need them, other libraries won't either, but that could be wrong).
  • Similarly, since postgraphile-core has dependencies on graphile-build, created a typings file for graphile-build, but only attempted to create typings needed by postgraphile-core (assumption same as for postgraphile-core).

I sprinkled some todo comments throughout these files to point out types I may have captured incorrectly (e.g., there were some tricky/impossible conversions between flow and TypeScript). That said, even the things I haven't marked should be verified and tested by someone who's intimately familiar with these projects.

@benjie
Copy link
Member

benjie commented May 5, 2018

Great work @devuxer; what you’ve done looks good from a cursory glance, but properly validating it would require more time and most likely a TypeScript project. Maybe others following this thread can try out your definitions and let us know 👍 or 👎

@devuxer
Copy link

devuxer commented May 5, 2018

@benjie, Sounds good, and I am trying it out in a TypeScript project myself at the moment. though I'm not touching much of the surface area of the API.

@sjmcdowall
Copy link

@devuxer -- I am also starting a TS project using Postgraphile and would love to try this. Do you have the steps needed to properly integrate your d.ts file(s) into a TS project? Thanks!

@devuxer
Copy link

devuxer commented Jun 6, 2018

@sjmcdowall, All you need to do is create a folder called "typings" where your server.ts file is, and place the d.ts files in that folder.

@sjmcdowall
Copy link

sjmcdowall commented Jun 13, 2018

@devuxer -- Just FYI -- I added (finally) all the typings to my project and I am getting some linting errors...
[ts]
Type '{ bodySizeLimit: string; graphiql: boolean; graphiqlRoute: string; graphqlRoute: string; }' is not assignable to type 'PostGraphileOptions'.
Object literal may only specify known properties, and 'bodySizeLimit' does not exist in type 'PostGraphileOptions'.

I think bodySizeLimit is legit as a string ??

2 --
Argument of type 'PostGraphileOptions' is not assignable to parameter of type 'PostGraphileOptions | undefined'.
Type 'import("postgraphile-core").PostGraphileOptions' is not assignable to type 'import("postgraphile").PostGraphileOptions'.
Types of property 'disableDefaultMutations' are incompatible.
Type 'string | undefined' is not assignable to type 'boolean | undefined'.
Type 'string' is not assignable to type 'boolean | undefined'.

Am I using the wrong type here?

// Finally set our misc. options
const options: PostGraphileOptions = {
  bodySizeLimit: '5MB',
  graphiql: true,
  graphiqlRoute: '/api/graphiql',
  graphqlRoute: '/api/graphql',
};

Each of the above options are being flagged as not valid .. ??

Cheers!

(Why don't you donate / add these to the @types thing?? These are pretty good starting points and better than anything else we don't have! :)

@sjmcdowall
Copy link

sjmcdowall commented Jun 14, 2018

BTW -- We found a problem in the postgraphile-core d.ts definition -- here is one that works
(the other version created compile errors:

src/lib/graphql.ts:29:3 - error TS2345: Argument of type 'PostGraphileOptions' is not assignable to parameter of type 'PostGraphileOptions | undefined'.
  Type 'import("postgraphile-core").PostGraphileOptions' is not assignable to type 'import("postgraphile").PostGraphileOptions'.
    Types of property 'disableDefaultMutations' are incompatible.
      Type 'string | undefined' is not assignable to type 'boolean | undefined'.
        Type 'string' is not assignable to type 'boolean | undefined'.

OH -- and FYI -- you need to add the following to your tsconfig.js to get it to work:

"lib": ["esnext"],  

This will resolve some nasty AsyncInterface errors in graphql itself ..

declare module "postgraphile-core" {
  import { Client, Pool } from "pg";
  import { Build, Context, Options, Plugin } from "graphile-build";

  export type mixed = {} | string | number | boolean | undefined | null

  type PostGraphileOptions = {
      dynamicJson?: boolean;
      classicIds?: boolean;
      disableDefaultMutations?: boolean;
      bodySizeLimit?: string;
      graphqlRoute?: string;
      graphiql?: boolean;
      graphiqlRoute?: string;
      nodeIdFieldName?: string;
      graphileBuildOptions?: Options;
      graphqlBuildOptions?: Options; // DEPRECATED!
      replaceAllPlugins?: Array<(builder: mixed) => {}>;
      appendPlugins?: Array<(builder: mixed) => {}>;
      prependPlugins?: Array<(builder: mixed) => {}>;
      skipPlugins?: Array<(builder: mixed) => {}>;
      jwtPgTypeIdentifier?: string;
      jwtSecret?: string;
      inflector?: any; // NO LONGER SUPPORTED!
      pgColumnFilter?: (mixed: any, Build: Build, Context: Context) => boolean; //
      viewUniqueKey?: string;
      enableTags?: boolean;
      readCache?: string;
      writeCache?: string;
      setWriteCacheCallback?: (fn: () => Promise<void>) => void;
      legacyRelations?: "only" | "deprecated";
      setofFunctionsContainNulls?: boolean;
      legacyJsonUuid?: boolean;
  };

  type PgConfig = Client | Pool | string;

  export const createPostGraphileSchema: (
      pgConfig: PgConfig,
      schemas: string | string[],
      options?: PostGraphileOptions
  ) => Promise<any>;

  export const watchPostGraphileSchema: (
      pgConfig: PgConfig,
      schemas: string | string[],
      options: PostGraphileOptions | undefined,
      onNewSchema: any
  ) => Promise<() => Promise<void>>;
}

@cdaringe
Copy link
Contributor

cdaringe commented Jul 7, 2018

TS can autogenerate typings for us. why not flip allowJs to false and just use the autogenerated typings? i was able to build the latest successfully w/ typings (i had to update one single export), and linked it successfully. it even found that i was incorrectly using watch instead of watchPg!

# change to tsconfig.json
    "declaration": true,                   /* Generates corresponding '.d.ts' file. */
    "declarationDir": "./build",
    "allowJs": false,

more testing to come...

@benjie
Copy link
Member

benjie commented Jul 7, 2018

PR welcome 👍

@sjmcdowall
Copy link

sjmcdowall commented Jul 7, 2018 via email

@cdaringe
Copy link
Contributor

cdaringe commented Jul 7, 2018

I was a little naive--there is a bit more to it, but not obscenely more. What to do about -core's typings is still unclear. I'm not sure if there's a way to ship those as part of this build effectively. I'd rather move those to -core itself personally

@sjmcdowall
Copy link

sjmcdowall commented Jul 7, 2018 via email

@cdaringe
Copy link
Contributor

cdaringe commented Jul 7, 2018

folks, critique and/or contribute to #794 if you can

@benjie
Copy link
Member

benjie commented Jul 21, 2018

#794 is now merged; we're now 100% TypeScript 🎉

For anyone looking for simple things to do to improve our typings (a great way of contributing to the project):

  • We should use NextHandleFunction from connect (import { NextHandleFunction } from 'connect';) in almost all places we're currently explicitly writing out (err?: Error) => void
  • We need to add TypeScript typings to postgraphile-core before we ship the next postgraphile; I'm considering just taking this file and adding it to postgraphile-core: https://github.com/graphile/postgraphile/blob/master/typings/postgraphile-core.d.ts
  • We should probably use a wider range for our typings files; however note that if we support later @types/koa than 2.0.44 then we'll need to solve the http2 issue
    "@types/graphql": "^0.8.2",
    "@types/jsonwebtoken": "<7.2.1",
    "@types/koa": "2.0.44",
    "@types/pg": "^7.4.10",
  • Lots of 'any' need replacing with explicit types

Please keep pull requests small and focussed; it's rare that I have 4 hours to drop on code review! (Each of these tasks should be a separate PR.)

@benjie
Copy link
Member

benjie commented Jul 21, 2018

Huge thanks to @cdaringe for undertaking this; I've been putting it off for a year because I had a suspicion how much work it would be!

@sjmcdowall
Copy link

sjmcdowall commented Jul 21, 2018 via email

@benjie
Copy link
Member

benjie commented Jul 21, 2018

It will be 👍 Need to add the postgraphile-core typings first. Hoping to get an @next out before Thursday.

@sjmcdowall
Copy link

sjmcdowall commented Jul 21, 2018 via email

@benjie
Copy link
Member

benjie commented Jul 21, 2018

Should be able to pull PostGraphileOptions from postgraphile in the new release 🎉

@sjmcdowall
Copy link

YAY!!!

@benjie
Copy link
Member

benjie commented Aug 15, 2018

[semi-automated message] We try and keep the open issues to actual issues (bugs, etc); this seems like more of a discussion right now, so I'm closing it but please feel free to keep discussing it below 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants