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

Add index signature to IResolvers and IDirectiveResolvers #1357

Merged
merged 2 commits into from
Feb 27, 2019

Conversation

ardatan
Copy link
Collaborator

@ardatan ardatan commented Feb 25, 2019

Related to #1133

@ardatan ardatan requested a review from dotansimha February 25, 2019 19:31
@kamilkisiela kamilkisiela self-requested a review February 26, 2019 08:48
Copy link
Collaborator

@kamilkisiela kamilkisiela left a comment

Choose a reason for hiding this comment

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

Few questions otherwise I give a 💚light

@@ -79,10 +79,10 @@ export interface IResolvers<TContext = {{{ getContext }}}> {
{{#each scalars}}
{{ convert name 'typeNames'}}{{#unless @root.config.strict}}?{{/unless}}: GraphQLScalarType;
{{/each}}
}
} & { [typeName: string] : never };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure it won't mark all properties as never?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah looks suspicious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can try it :)

@dotansimha
Copy link
Owner

Please note @kamilkisiela 's notes, and fix CI.

@ardatan
Copy link
Collaborator Author

ardatan commented Feb 26, 2019

@dotansimha Fixed 👍

@dotansimha dotansimha merged commit b0c24ea into master Feb 27, 2019
@dotansimha dotansimha deleted the fix/iresolvers branch February 27, 2019 06:44
@SimenB
Copy link
Contributor

SimenB commented Mar 1, 2019

How are you supposed to implement this interface?

See minimal example: https://github.com/SimenB/ts-codegen-resolver
It has a type error when trying to create an IResolver

What am I doing wrong?

@domasx2
Copy link

domasx2 commented Mar 1, 2019

on TS 3.3.333 these interfaces are now unimplementable, 'not assignable to type never' error.
Had to roll back to 0.17.0

@ardatan
Copy link
Collaborator Author

ardatan commented Mar 4, 2019

Fixed in #1389
Could you try with these canary versions?

Successfully published:
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - graphql-codegen-typescript-graphql-files-modules@0.18.0-alpha.69f43292
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - graphql-codegen-graphql-files-typescript-modules@0.18.0-alpha.69f43292
 - [email protected]
 - graphql-codegen-typescript-mongodb-template@0.18.0-alpha.69f43292
 - graphql-codegen-typescript-react-apollo-template@0.18.0-alpha.69f43292
 - graphql-codegen-typescript-resolvers-template@0.18.0-alpha.69f43292
 - [email protected]
lerna success publish finished

@SimenB
Copy link
Contributor

SimenB commented Mar 4, 2019

Thanks! It almost works 🙂

image

export type IDirectiveResolvers<Result> = {
  client: ClientDirectiveResolver<Result>;
  skip: SkipDirectiveResolver<Result>;
  include: IncludeDirectiveResolver<Result>;
  deprecated: DeprecatedDirectiveResolver<Result>;
} & { [directiveName: string]: DirectiveResolverFn<any, any, TContext> };

@ardatan
Copy link
Collaborator Author

ardatan commented Mar 4, 2019

Could you try again with 0.18.0-alpha.69f43292?

@SimenB
Copy link
Contributor

SimenB commented Mar 4, 2019

EDIT: Ignore the error, see edit at bottom.

The generated code now compiles! I'm having issues passing TContext through, though. I have IResolvers<{ cache: InMemoryCache }>, but end up with this error:

image

image

The field cache is there, but its type is set to any

image


EDIT: Wait no, that's my fault! I do Omit<IResolvers<{ cache: InMemoryCache }>, 'Query'>, and it seems Omit from utility-types loses the type. Removing the omit resolves the issue.

@ardatan
Copy link
Collaborator Author

ardatan commented Mar 4, 2019

@SimenB So, now is it working as expected?

@SimenB
Copy link
Contributor

SimenB commented Mar 4, 2019

Yes! 🙂

@dotansimha dotansimha mentioned this pull request Mar 5, 2019
38 tasks
@ctrlplusb
Copy link

on TS 3.3.333 these interfaces are now unimplementable, 'not assignable to type never' error.

The 0.19.0-alpha.3973cc8b canary release resolved this for myself.

@SimenB
Copy link
Contributor

SimenB commented Mar 10, 2019

Same here, eagerly awaiting the release so we can upgrade

kamilkisiela pushed a commit that referenced this pull request Mar 14, 2019
Related: #1350 

Planned in this PR:
- [x] Move all common code from `flow` to a common package.
- [x] Make sure `flow` plugin works.
- [x] Add the latest changes of `flow` plugin.
- [x] Create `typescript` package with the same features as `flow` package.
- [x] Write tests for `typescript` package.
 
- [x] Move all common code from `flow-documents` to a common package.
- [x] Make sure `flow-documents` plugin works.
- [x] Add the latest changes of `flow-documents` plugin.
- [x] Create `typescript-documents` package with the same features as `flow-documents` package.
- [x] Write tests for `typescript-documents` package.
- [x] Move all common code from `flow-resolvers` to a common package.
- [x] Make sure `flow-resolvers` plugin works.
- [x] Add the latest changes of `flow-resolvers` plugin.
- [x] Create `typescript-resolvers` package with the same features as `flow-resolvers` package.
- [x] Write tests for `typescript-resolvers` package.
- [x] Add missing features from previous `typescript` package.
- [x] Add missing features from previous `typescript-documents` package.
- [x] Add missing features from previous `typescript-resolvers` package.
- [x] Add introspection types @kamilkisiela  #1406
- [x] Implement `namingConvention` again @kamilkisiela  #1407
- [x] Fix issue with prefix @kamilkisiela #1407
- [x] Apply this: #1357 @ardatan 
- [x] Refactor `react-apollo` @dotansimha 
- [x] Refactor `apollo-angular` @dotansimha 
- [x] Apply fix to Flow plugins: #1413 @kamilkisiela 
- [x] Refactor `typescript-mongo` @dotansimha  
- [x] Refactor `stencil-apollo` @ardatan
- [x] Remove Handlebars compiler package from all plugins, and add support for tree shaking. @ardatan 
- [x] Make sure to update packages in `gql-gen init`  @kamilkisiela 
- [x] Rename `XXX-documents` to `XXX-operations`
- [x] Move all flatten utils from `core` to `plugin-helpers` and clean `core` package. @kamilkisiela 
- [x] Deprecate `buildTemplateContext` and `flattenTypes`. @kamilkisiela 
- [x] Repo cleanup
- [x] Update README @kamilkisiela  
- [x] Update docs @kamilkisiela  
- [x] Write docs about writing plugins with visitor @kamilkisiela 
- [x] Update examples
- [x] Change packages names @dotansimha
@Pruxis
Copy link

Pruxis commented Mar 16, 2019

When I am using 0.19.0-alpha.3973cc8b canary version I am not able to generate my regular types. But the resolvers seem to be correct.

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

Successfully merging this pull request may close these issues.

7 participants