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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typings to package. #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mathieutu
Copy link
Contributor

Hi,
Here are some typescript typing to resolve #7.
This is my first typings, and I'm a total noob in TS, so it's totally opened for expertise, to learn.

The only thing I can say is, like it's done, it works! 馃槄
I'm using TS on my project, and it correctly recognize all the types.

Thanks for your work on this package!

@coveralls
Copy link

coveralls commented Jul 26, 2018

Pull Request Test Coverage Report for Build 53

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 35: 0.0%
Covered Lines: 221
Relevant Lines: 221

馃挍 - Coveralls

@b1f6c1c4 b1f6c1c4 added enhancement New feature or request help wanted Extra attention is needed labels Jul 27, 2018
@b1f6c1c4
Copy link
Owner

@b4dnewz @Xilesun Could you two help review this PR?

@Xilesun
Copy link

Xilesun commented Jul 27, 2018

@b1f6c1c4 It seems work well in my project.
@mathieutu Good job!

I think you can make a PR on DefinitelyTyped, so that we can use it conveniently by npm install @types/graphql-advanced-projection later. 馃憤

@mathieutu
Copy link
Contributor Author

@Xilesun why sould we push that on @types if @b1f6c1c4 is ok to merge? DefinitelyTyped is just for repos which don't have typings in them.

Quoting:

When a package bundles its own types, types should be removed from DefinitelyTyped to avoid confusion.

@Xilesun
Copy link

Xilesun commented Jul 27, 2018

@mathieutu Your opinion is right.

type gqlProjection = (config: RawConfig) => { project: ProjectMethod, resolvers: Resolver };
type prepareConfig = (config: RawConfig) => PreparedConfig;
type genProjection = (config: PreparedConfig) => ProjectMethod;
type genResolvers = (config: PreparedConfig) => Resolver[]
Copy link
Owner

Choose a reason for hiding this comment

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

genResolvers does NOT give GraphQLFieldResolver<,>.
It produces:

{
  User: {
    userId: (parent, args, context, info) => parent._id,
    field1: (parent, args, context, info) => parent.mongoA,
  },
}

while GraphQLFieldResolver<,> is essentially the type of (parent, args, context, info) => any.

Copy link
Contributor Author

@mathieutu mathieutu Jul 27, 2018

Choose a reason for hiding this comment

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

Indeed, I've missed a level.

@@ -0,0 +1,20 @@
import { GraphQLFieldResolver, GraphQLResolveInfo } from 'graphql';
Copy link
Owner

Choose a reason for hiding this comment

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

@mathieutu Would you please use a linter (such as tslint) to lint the code, enforcing a consistant coding style? I found that there are inconsistent spaces and semicolons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript request
4 participants