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

use getDescription from graphql package #672

Merged
merged 31 commits into from
Jul 19, 2018
Merged

use getDescription from graphql package #672

merged 31 commits into from
Jul 19, 2018

Conversation

divyenduz
Copy link
Contributor

@divyenduz divyenduz commented Mar 9, 2018

WIP: Not ready to be merged yet. Still need to expose getDescription at proper path with proper typings in @types/graphql. I am working on it - already working and tested on local by changing directly in node_modules, working on a PR.

Raising a PR because this is one of my first works in TS and I want to be sure that I am not doing something wrong :)

Based on the discussion, I think we are supposed to get the getDescription function from index. Corresponding typings in @types/graphql look outdated, working on bringing them up to speed with graphql-js.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

@divyenduz
Copy link
Contributor Author

Prepared a PR in @types/graphql - DefinitelyTyped/DefinitelyTyped#24169

We need to wait for it to be merged and then bump the version of @types/graphql in this repository for the tests to start passing on this one.

Please let me know what else I can do to get this merged or if I am doing something wrong :)

Thanks!

@divyenduz divyenduz changed the title use getDescription from gaphql package use getDescription from graphql package Mar 9, 2018
@divyenduz
Copy link
Contributor Author

divyenduz commented Mar 16, 2018

The @types/graphql PR got merged and release. However, I am now running into this

src/schemaGenerator.ts(248,7): error TS2365: Operator '===' cannot be applied to types '"OperationDefinition" | "FragmentDefinition" | "SchemaDefinition" | "ScalarTypeDefinition" | "Obj...' and '"TypeExtensionDefinition"'.

Which lead me to this, but I think this is already done.

I am new to TS and can use a comment on how to resolve the above error. I understand that we cannot compare a union(?) type with string type with === but should I change that to a switch case or something? We can use === but the names have changed. Reading this and related work to get more context.

@stubailo Please advice as you have much more context of this situation than me.

@divyenduz
Copy link
Contributor Author

divyenduz commented Mar 20, 2018

I attempted to fix the above issue with this commit but it has broken GRAPHQL_VERSION 0.11 and 0.12 because they don't have the getDescription function which is expected I think.

Advice on how should I proceed?

@divyenduz
Copy link
Contributor Author

divyenduz commented Mar 20, 2018

From CHANGELOG

### v2.14.0

Update to add support for graphql@0.12, and drop versions before 0.11 from the peer dependencies list. The graphql package has some breaking changes you might need to be aware of, but there aren't any breaking changes in graphql-tools itself, or common usage patterns, so we are shipping this as a minor version. We're also running tests on this package with both graphql@0.11 and graphql@0.12 until we confirm most users have updated.

How do we confirm if 0.11/0.12 can be removed now?

In any case, I am preparing a commit removing them for further discussion 👍

@freiksenet
Copy link
Contributor

I think this is okay to merge if we are okay to drop older versions. CC @benjamn

@benjamn
Copy link
Contributor

benjamn commented Apr 10, 2018

We will be sure to merge this once we're ready to drop support for 0.11 and 0.12 (thanks @freiksenet), likely when we release graphql-tools@3 with schema transforms (#527).

@benjamn
Copy link
Contributor

benjamn commented Apr 23, 2018

@divyenduz Can you rebase this against master now that #527 has been merged? I would like to get this into graphql-tools@3.0.0 if possible!

@divyenduz
Copy link
Contributor Author

@benjamn This is done, sorry for the delay, was traveling and just got access to a laptop. I understand tha 3.0.0 is already out, I think we can get this in the next but do we might need at least a minor bump as we are dropping support in this?

@divyenduz
Copy link
Contributor Author

@benjamn : Would it be possible to merge this anytime soon? Thanks.

@divyenduz divyenduz closed this Jun 23, 2018
@divyenduz divyenduz reopened this Jun 23, 2018
@divyenduz
Copy link
Contributor Author

Closed and reopened PR to re-trigger the build. I think it had failed for some reason with travis unrelated to this PR.

@stubailo stubailo merged commit c77e3c2 into ardatan:master Jul 19, 2018
@stubailo
Copy link
Contributor

@divyenduz thanks a lot! Looks like we don't support 0.12 anymore anyway, so that made things a lot simpler. Thanks!

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.

4 participants