FoalTS / foal

Full-featured Node.js framework, with no complexity. 🚀 Simple and easy to use, TypeScript-based and well-documented.
https://foalts.org/
MIT License
1.9k stars 140 forks source link

Seemingly incomplete GraphQL documentation #614

Closed Edo78 closed 4 years ago

Edo78 commented 4 years ago

I just tryed to play with graphql so I created a new foalts app and followed the instruction on https://foalts.gitbook.io/docs/topic-guides/api/graphql

Sadly I got some errors

❯ npm run develop                                                                                                                                                                                                        ─╯

> foal-graphql@0.0.0 develop /home/edo/test/foal-graphql
> npm run build:app && concurrently "npm run build:app:w" "npm run start:w"

> foal-graphql@0.0.0 build:app /home/edo/test/foal-graphql
> copy-cli "src/**/*.html" build && tsc -p tsconfig.app.json

node_modules/graphql/subscription/subscribe.d.ts:44:3 - error TS2304: Cannot find name 'AsyncIterableIterator'.

44   AsyncIterableIterator<ExecutionResult<TData>> | ExecutionResult<TData>
     ~~~~~~~~~~~~~~~~~~~~~

node_modules/graphql/subscription/subscribe.d.ts:57:3 - error TS2304: Cannot find name 'AsyncIterableIterator'.

57   AsyncIterableIterator<ExecutionResult<TData>> | ExecutionResult<TData>
     ~~~~~~~~~~~~~~~~~~~~~

node_modules/graphql/subscription/subscribe.d.ts:86:12 - error TS2304: Cannot find name 'AsyncIterable'.

86 ): Promise<AsyncIterable<any> | ExecutionResult<TData>>;
              ~~~~~~~~~~~~~

Found 3 errors.

This happens because tsconfig.json lacks the lib esnext.asynciterable (adding it solves those problems). I double checked to be sure but I can't find any reference in the doc.

If needed I can create a PR to update the doc. Let me know

LoicPoullain commented 4 years ago

Hi @Edo78

Thank you for pointing this out.

It looks like new versions of the graphql package need that setting. This is unfortunate because this appeared between two minor versions (14.3.0 works but 14.5.8 does not).

Yes, feel free to open a PR. I think the best place to add this is just after the npm install command in https://github.com/FoalTS/foal/blob/master/docs/api-section/graphql.md. Maybe something like this:

tsconfig.json

{
  "compilerOptions": {
    ...
    "lib": [
      ...
      "ESNext.AsyncIterable"
    ]
  }
  ...
}

Thanks!

Edo78 commented 4 years ago

I also noticed that the actual graphql docs doesn't talk about typeorm & type-graphql integration (unless I missed it). In my opinion it should have a prominent role and I start to write some code experimenting on them. If you will I can add something about this argument in this very same PR (or in another one to keep a better separation).

LoicPoullain commented 4 years ago

@Edo78 the documentation actually mentions how to integrate TypeGraphQL the GraphQLController: https://foalts.gitbook.io/docs/topic-guides/api/graphql#using-typegraphql .

I would prefer not to add more documentation on TypeGraphQL and TypeORM because they both have their own docs with many examples and I think this would be a bit out of the scope of the framework documentation.

Edo78 commented 4 years ago

Ok,I may have been to vague. Let me rephrase it. I see the section about type-graphql but IMO is missing the point for a meaningful integration with the framework itself.

You wrote section about having a graphql schema completely separated from the db schema but this implies that who want to have the same schema for both must replicate it. Using type-graphql to annotate the typeorm schema can simplify this procedure. For example the todo entity from the example can be written as

import { Column, Entity, PrimaryGeneratedColumn } from 'typeorm';
import { Field, ObjectType } from 'type-graphql';

@Entity()
@ObjectType()
export class Todo {

  @PrimaryGeneratedColumn()
  @Field()
  id: number;

  @Column()
  @Field()
  text: string;
}

I may be wrong but having a graphql schema resembling the db ones is a common practice and so it should have be at least an example. Just to provide users a way to understand how this technology could be integrated in the framework.

Obviously this is just my 2 cents.

LoicPoullain commented 4 years ago

Resolved with #623 and #624