Add initial types for fgraphql #469

NickBolles · 2018-11-20T03:36:15Z

Summary

Add initial version of types for fgraphql. This was erroring (for some reason, not quite sure why though) in my project, which was generated by feathers+ cli. So I thought I'd get these started.

Tell us about the problem your pull request is solving.
Are there any open issues that are related to this?
TS for keep fgraphql #467
Is this PR dependent on PRs in other repos?

Other Information

There are a few enhancements that we could do still, to query, runtime, parse function return and possibly resolvers (a generic type for ResolverMap)

Docs issue #466

question: Why is there an async version and a non-async version. As far as I can tell they are actually the same implementation, with different keywords

eddyystop · 2018-11-20T13:00:09Z

Thanks for the effort. We do need typings for fgraphql. https://medium.com/@eddyystop/fgraphql-feathers-graphql-hook-38faee75dd1

Unfortunately the tests are not passing.

Setting APT mirror in /etc/apt/sources.list: http://us-east-1.ec2.archive.ubuntu.com/ubuntu/
git.checkout
1.57s$ git clone --depth=50 https://github.com/feathers-plus/feathers-hooks-common.git feathers-plus/feathers-hooks-common
Cloning into 'feathers-plus/feathers-hooks-common'...
remote: Enumerating objects: 795, done.
remote: Counting objects: 100% (795/795), done.
remote: Compressing objects: 100% (443/443), done.
remote: Total 795 (delta 415), reused 629 (delta 350), pack-reused 0
Receiving objects: 100% (795/795), 326.04 KiB | 14.82 MiB/s, done.
Resolving deltas: 100% (415/415), done.
$ cd feathers-plus/feathers-hooks-common
1.03s$ git fetch origin +refs/pull/469/merge:
remote: Enumerating objects: 50, done.
remote: Counting objects: 100% (42/42), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 10 (delta 5), reused 8 (delta 4), pack-reused 0
Unpacking objects: 100% (10/10), done.
From https://github.com/feathers-plus/feathers-hooks-common

branch refs/pull/469/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD
nvm.install
4.31s$ nvm install node
Downloading and installing node v11.2.0...
Downloading https://nodejs.org/dist/v11.2.0/node-v11.2.0-linux-x64.tar.xz...
Computing checksum with sha256sum
Checksums matched!
Now using node v11.2.0 (npm v6.4.1)
$ node --version
v11.2.0
$ npm --version
6.4.1
$ nvm --version
0.33.11
4.97s$ npm ci
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing.
npm ERR!
npm ERR!
npm ERR! Invalid: lock file's @types/graphql@0.11.8 does not satisfy @types/graphql@^14.0.3
npm ERR!
npm ERR! A complete log of this run can be found in:
npm ERR! /home/travis/.npm/_logs/2018-11-20T03_41_10_109Z-debug.log
The command "eval npm ci " failed. Retrying, 2 of 3.
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing.
npm ERR!
npm ERR!
npm ERR! Invalid: lock file's @types/graphql@0.11.8 does not satisfy @types/graphql@^14.0.3
npm ERR!
npm ERR! A complete log of this run can be found in:
npm ERR! /home/travis/.npm/_logs/2018-11-20T03_41_11_764Z-debug.log
The command "eval npm ci " failed. Retrying, 3 of 3.
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing.
npm ERR!
npm ERR!
npm ERR! Invalid: lock file's @types/graphql@0.11.8 does not satisfy @types/graphql@^14.0.3
npm ERR!
npm ERR! A complete log of this run can be found in:
npm ERR! /home/travis/.npm/_logs/2018-11-20T03_41_13_450Z-debug.log
The command "eval npm ci " failed 3 times.
The command "npm ci " failed and exited with 1 during .
Your build has been stopped.

NickBolles · 2018-11-20T14:41:23Z

@eddystop I've gotta update the package lock.json. I'll do that in a few hours.

NickBolles · 2018-11-20T18:01:27Z

@eddyystop Should be fixed now. Took a few tries though lol. I don't know why vscode wasn't catching these things. Is it because there isn't a tslint.json?

eddyystop · 2018-11-20T18:57:20Z

@j2L4e please confirm this follows the standards the rest of typings follow.

types/index.d.ts

j2L4e · 2018-11-20T20:26:14Z

types/index.d.ts

+        [i: string]: (parent: any, args: any, content: any, ast: any) => any;
+    };
+    Query: {
+        [i: string]: (parent: any, args: any, content: any, ast: any) => any;


I think these anys could be stricter, but I can imagine that it may be hard to get right. Just thinking. Tightening these later would most likely be a breaking change.

Parent, args, content, ast will always be objects. I guess its possible some custom code may make context,ast undefined or perhaps null.

j2L4e · 2018-11-20T20:29:46Z

@NickBolles thanks for adding the tests and fixing the linting errors. dtslint is pretty strict and can be a pain sometimes.

eddyystop · 2018-12-01T13:18:05Z

@jsl4e do we want to do anything about "Parent, args, content, ast will always be objects. I guess its possible some custom code may make context,ast undefined or perhaps null."? Or should I merge?

j2L4e · 2018-12-01T15:51:16Z

Sounds like any is the right thing here if it's possible to change the type with custom code. I guess args could at least be type any[].

eddyystop · 2018-12-01T16:53:00Z

Should I merge?

NickBolles · 2018-12-02T19:58:49Z

I missed this conversation before. I think we should make the two changes that j2l4e suggested. I can do those quick later tonight I think.

I haven't used fgraphql at all since doing this PR, so this hasn't really been in my mind. Sorry about the delay!

j2L4e · 2018-12-02T20:02:47Z

That'd be great, go ahead: )

j2L4e · 2018-12-04T12:28:44Z

@NickBolles FGraphqlOptions is missing the options property. Details can be found in the article about half way down, following "We now understand enough to review the options parameter in the fgraphql call."
~~Could you add the needed typings?~~ (edit: I'll need to make some more changes to sort out some issues with services/graphql/graphql.interfaces.ts, so don't bother :))
I'm working on feathers-plus/generator-feathers-plus#130 point 2 and the missing options field is blocking.
I see they're all actually there but you accidentally flattened the interface. It should look like this

export interface FGraphqlOptions {
    recordType: string;
    schema: string;
    resolvers: ResolversObject | ResolversFunction;
    query: Query | SyncContextFunction<Query>;
    runTime: any;
    parse: typeof parse;
    options?: {
        inclAllFieldsServer?: boolean;
        inclAllFieldsClient?: boolean;
        inclAllFields?: boolean;
        inclJoinedNames?: boolean;
        extraAuthProps?: string[];
        skipHookWhen?: SyncContextFunction<boolean>;
    }
}

NickBolles · 2018-12-04T17:29:27Z

@j2L4e @eddyystop Why is there an async version and a non-async version? As far as I can tell they are actually the same implementation, one just explicitly returns a promise, while the other is an async function which also returns a promise, just via async/await.

j2L4e · 2018-12-04T18:30:05Z

@NickBolles sync/async version of what? I'll be back tomorrow morning

NickBolles · 2018-12-04T18:50:05Z

@j2L4e of the fgraphql hook

https://github.com/feathers-plus/feathers-hooks-common/blob/master/lib/services/fgraphql.js

vs

https://github.com/feathers-plus/feathers-hooks-common/blob/master/lib/services/fgraphql-async.js

eddyystop · 2018-12-04T18:54:28Z

We will remove fgraphql-async because batchloader can only work with promises. I had to rewrite the original async back into promises. 🤥

NickBolles · 2018-12-04T19:36:23Z

Gotcha, that makes sense!

eddyystop · 2018-12-26T13:16:37Z

I'm losing track of outstanding TS issues. Is this one resolved?

I'm asking because I have to write the next article on populate hooks but don't want to do that if there are issues with it in TS.

NickBolles · 2018-12-26T16:18:59Z

Looks done to me

NickBolles added 2 commits November 19, 2018 21:22

Add initial types for fgraphql

25d1c67

add app parameter type

18add8b

NickBolles added 3 commits November 20, 2018 11:04

Add tests and strengthen types some

3dd9e9d

Fix linting issues

bcf93ec

fix more linting issues

ba2dee0

j2L4e suggested changes Nov 20, 2018

View reviewed changes

eddyystop merged commit 20d9a24 into feathersjs-ecosystem:master Dec 2, 2018

NickBolles mentioned this pull request Dec 4, 2018

update names and fix the options property #481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial types for fgraphql #469

Add initial types for fgraphql #469

NickBolles commented Nov 20, 2018

eddyystop commented Nov 20, 2018

NickBolles commented Nov 20, 2018

NickBolles commented Nov 20, 2018

eddyystop commented Nov 20, 2018

j2L4e Nov 20, 2018

eddyystop Nov 20, 2018

j2L4e commented Nov 20, 2018

eddyystop commented Dec 1, 2018

j2L4e commented Dec 1, 2018

eddyystop commented Dec 1, 2018

NickBolles commented Dec 2, 2018

j2L4e commented Dec 2, 2018

j2L4e commented Dec 4, 2018 •

edited

NickBolles commented Dec 4, 2018 •

edited

j2L4e commented Dec 4, 2018

NickBolles commented Dec 4, 2018

eddyystop commented Dec 4, 2018

NickBolles commented Dec 4, 2018

eddyystop commented Dec 26, 2018 •

edited

NickBolles commented Dec 26, 2018

Add initial types for fgraphql #469

Add initial types for fgraphql #469

Conversation

NickBolles commented Nov 20, 2018

Summary

Other Information

eddyystop commented Nov 20, 2018

NickBolles commented Nov 20, 2018

NickBolles commented Nov 20, 2018

eddyystop commented Nov 20, 2018

j2L4e Nov 20, 2018

Choose a reason for hiding this comment

eddyystop Nov 20, 2018

Choose a reason for hiding this comment

j2L4e commented Nov 20, 2018

eddyystop commented Dec 1, 2018

j2L4e commented Dec 1, 2018

eddyystop commented Dec 1, 2018

NickBolles commented Dec 2, 2018

j2L4e commented Dec 2, 2018

j2L4e commented Dec 4, 2018 • edited

NickBolles commented Dec 4, 2018 • edited

j2L4e commented Dec 4, 2018

NickBolles commented Dec 4, 2018

eddyystop commented Dec 4, 2018

NickBolles commented Dec 4, 2018

eddyystop commented Dec 26, 2018 • edited

NickBolles commented Dec 26, 2018

j2L4e commented Dec 4, 2018 •

edited

NickBolles commented Dec 4, 2018 •

edited

eddyystop commented Dec 26, 2018 •

edited