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 a way to disable comments as descriptions backcompat #544

Closed
stubailo opened this issue Dec 18, 2017 · 9 comments · Fixed by #559
Closed

Add a way to disable comments as descriptions backcompat #544

stubailo opened this issue Dec 18, 2017 · 9 comments · Fixed by #559
Labels
docs Focuses on documentation changes easy enhancement help wanted Extra attention is needed

Comments

@stubailo
Copy link
Contributor

stubailo commented Dec 18, 2017

In [email protected], the preferred way to pass descriptions is with strings, which can now be multiline:

"""
This is a long description
"""
type MyType {
  """
  This can be indented along with the field
   
  Multiple lines here
  """
  field(args: [String]): OtherType
}

We should update examples to encourage this approach over the comment-based one. Also, we should add tests for it to ensure it continues to work the way we expect.

This would go in the docs here: https://www.apollographql.com/docs/graphql-tools/generate-schema.html#descriptions

Also, we should probably add a flag to various functions to disable comments-as-docs, so that people can ensure they have migrated and we can remove that feature in the next major version.

@stubailo stubailo added docs Focuses on documentation changes easy enhancement help wanted Extra attention is needed labels Dec 18, 2017
@email2vimalraj
Copy link
Contributor

email2vimalraj commented Dec 18, 2017

So if I understand it correctly, it involves the following tasks:

  1. Update the doc
  2. Add test
  3. Also, we should probably add a flag to various functions to disable comments-as-docs, so that people can ensure they have migrated and we can remove that feature in the next major version.

I'm afraid that I understood the point # 3 correctly.

I can start of with the point # 1, if you are fine with it.

@stubailo
Copy link
Contributor Author

Yeah, starting with (1) is a great idea! I like having the test as well just so we make sure the docs aren't going to break :]

I can write a more detailed description of (3) in a bit, thank you for jumping in!

@email2vimalraj
Copy link
Contributor

@stubailo : FYI, I'm currently working on this. So far, I've updated the docs and currently working on the test part.
It would be great if you can give me some pointer for the (3), so that I can send a single PR if possible.

@stubailo
Copy link
Contributor Author

stubailo commented Dec 20, 2017

Let's do 2 PRs - I think many small PRs is much easier to work with than one big one for me!

For the 3rd, there's this variable around the codebase called backcompatOptions which passes in a flag to turn on comment descriptions. I think we want an option to makeExecutableSchema, mergeSchemas, and maybe other functions I'm forgetting that make sure that backcompat is turned off. So we can say something like:

When using graphql-tools before version X, you used comments to add descriptions to types, fields, and arguments. In the newer versions of the GraphQL specification, you should instead use block strings (...etc...).

To check if you have migrated everything, try adding the disableDocCommentsBackcompat: true flag to makeExecutableSchema. If your app runs, then you've migrated everything correctly! In version 3.0 of graphql-tools this will be the new default.

freiksenet pushed a commit that referenced this issue Jan 3, 2018
…attern in new GraphQL SDL (#559)

* #64 - Update docs and tests to reflect new strings-as-descriptions pattern in new GraphQL SDL

* Make tests compatible with graphql v0.11

* Fix test errors

* Remove extra space

* Fix review comments
@email2vimalraj
Copy link
Contributor

I believe this issue shouldn't be closed since there is one more pending to be raised. Can we reopen this?

@stubailo stubailo changed the title Update docs to reflect new strings-as-descriptions pattern in new GraphQL SDL Add a way to disable comments as descriptions backcompat Jan 3, 2018
@stubailo
Copy link
Contributor Author

stubailo commented Jan 3, 2018

Yeah, I think it was automatically closed from the PR being merged :]

@stubailo stubailo reopened this Jan 3, 2018
freiksenet added a commit that referenced this issue Feb 15, 2018
Squashed commit of the following:

commit a108877910e0556759f92e9acab4643c63be13ff
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 31 16:05:00 2018 +0200

    First transforms

commit 334802c
Merge: 99884b6 e9c0eb5
Author: Mikhail Novikov <[email protected]>
Date:   Fri Jan 26 12:27:57 2018 +0200

    Merge remote-tracking branch 'origin/master' into next-api

commit 99884b6
Author: Mikhail Novikov <[email protected]>
Date:   Fri Jan 26 11:57:15 2018 +0200

    Restored valid fragments

commit f1b2432
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 25 14:55:57 2018 +0200

    Progress :P

commit db2806a
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 25 13:35:35 2018 +0200

    Fragment replacement

commit e9c0eb5
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 24 18:28:12 2018 +0200

    v2.19.0 (#594)

commit e33c215
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 24 16:31:37 2018 +0200

    Progress

commit 464670f
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 23 15:03:40 2018 +0200

    More progress

commit 22d2295
Author: Sebastian Richter <[email protected]>
Date:   Tue Jan 23 12:45:49 2018 +0100

    Also recreate astNode for fields (#580)

    * Also recreate astNode for fields

    In a [previous commit](fd9f626) we added the `astNode` property in the `reacreateCompositeType` function. That resulted in cache control working with schema stitching but only for GraphQL Types. By recreating the `astNode` prop also in `fieldToFieldConfig` cache control also works for fields. This is required for caching fields and hence queries.

    * Add ast to input field node too

commit 03ad432
Author: Renato Benkendorf <[email protected]>
Date:   Tue Jan 23 08:35:00 2018 -0200

    Fix resolvers to accept and move forward args with zero or false values (#586)

    Fixing the args with zero value or false

commit 72ac16f
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 23 11:37:02 2018 +0200

    Upgrade remap-istanbul (#590)

commit 5420557
Merge: 24a0f3f 3ef557a
Author: Sashko Stubailo <[email protected]>
Date:   Mon Jan 22 11:55:07 2018 -0800

    Merge pull request #584 from shackpank/double_deps

    Remove graphql-subscriptions from devDependencies

commit 3ef557a
Author: Ollie Buck <[email protected]>
Date:   Fri Jan 19 14:40:03 2018 +0000

    Remove graphql-subscriptions from devDependencies

commit 7e4efe9
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 18 16:38:13 2018 +0200

    Progress

commit 24a0f3f
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 10 12:36:43 2018 +0200

    v2.18.0 (#574)

commit 966e102
Author: Amit-A <[email protected]>
Date:   Wed Jan 10 02:53:56 2018 -0500

    Fixing broken links (#573)

commit 94b8f68
Author: Pascal Kaufmann <[email protected]>
Date:   Tue Jan 9 12:51:53 2018 +0100

    Fix fragment filtering edge case when a type implements multiple interfaces (#546)

    * add failing test for multi interface issue

    * interface / interface always implements an abstract type

    * changelog

    * the linter always should be happy

    * Update CHANGELOG.md

commit 608414b
Author: Tim Griesser <[email protected]>
Date:   Tue Jan 9 06:15:12 2018 -0500

    Allow IEnumResolver values to be number type (#568)

    * Allow IEnumResolver values to be number type

commit 6177034
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 9 11:26:04 2018 +0200

    v2.17.0 (#571)

commit fd9f626
Author: Sebastian Richter <[email protected]>
Date:   Tue Jan 9 09:25:16 2018 +0100

    Include astNode in schema recreation (#569)

    * Update schemaRecreation.ts

    `mergeSchemas` does not set `astNode` properties, when recreating types.
    But `CacheControlExtension.willResolveField` uses the `astNode` property in order to get the `cacheControl` directives.

commit 4f489d3
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 3 15:40:21 2018 +0200

    v2.16.0 (#564)

commit c4c5d91
Author: VimalRaj Selvam <[email protected]>
Date:   Wed Jan 3 19:00:16 2018 +0530

    #544 - Update docs and tests to reflect new strings-as-descriptions pattern in new GraphQL SDL (#559)

    * #64 - Update docs and tests to reflect new strings-as-descriptions pattern in new GraphQL SDL

    * Make tests compatible with graphql v0.11

    * Fix test errors

    * Remove extra space

    * Fix review comments

commit 66d58bb
Author: Alvis Tang <[email protected]>
Date:   Wed Jan 3 13:25:59 2018 +0000

    fix: correct the dependency issue in typescript caused by #445 (#561)

    fix #472

commit 342bece
Author: Johannes Schickling <[email protected]>
Date:   Wed Jan 3 14:21:35 2018 +0100

    Add subscriptions support to `makeRemoteExectuableSchema` (#563)

    Added subscriptions support to makeRemoteExectuableSchema

commit 8e8e348
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 2 14:50:06 2018 +0200

    v2.15.0 (#562)

commit f2905ac
Author: Johannes Schickling <[email protected]>
Date:   Tue Jan 2 13:40:56 2018 +0100

    Add document validation to `delegateToSchema` (#551)

    * Add document validation to `delegateToSchema`

commit 59592e1
Merge: 3e24c69 3612216
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 10:02:28 2017 -0800

    Merge pull request #556 from enaqx/patch-4

    doc: fix Generating schema URL on Mocking page

commit 3612216
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 10:00:16 2017 -0800

    Update mocking.md

commit 3e24c69
Merge: 7089bbf 42d180d
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:58:31 2017 -0800

    Merge pull request #557 from tbroadley/fix-typos

    docs: fix typos

commit 42d180d
Merge: 5130cf6 7089bbf
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:57:11 2017 -0800

    Merge branch 'master' into fix-typos

commit 7089bbf
Merge: 3a58286 5048434
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:52:44 2017 -0800

    Merge pull request #552 from mklopets/patch-2

    Fix link to Apollo Link docs

commit 5048434
Author: Marko Klopets <[email protected]>
Date:   Thu Dec 28 14:23:07 2017 +0200

    Re-fix link

commit 5130cf6
Author: Thomas Broadley <[email protected]>
Date:   Mon Dec 25 17:45:44 2017 -0500

    docs: fix typos

commit 80f2f4c
Author: Nick Raienko <[email protected]>
Date:   Mon Dec 25 09:19:03 2017 +0200

    doc: fix Generating schema URL on Mocking page

commit e295019
Author: Marko Klopets <[email protected]>
Date:   Thu Dec 21 13:18:59 2017 +0200

    Fix link to Apollo Link docs

commit 3a58286
Merge: 259f22e 971e96e
Author: Sashko Stubailo <[email protected]>
Date:   Wed Dec 20 11:26:55 2017 -0800

    Merge pull request #548 from apollographql/create-2.14.1

    v2.14.1

commit 971e96e
Author: Mikhail Novikov <[email protected]>
Date:   Wed Dec 20 12:44:14 2017 +0200

    v2.14.1

commit 259f22e
Author: Mikhail Novikov <[email protected]>
Date:   Wed Dec 20 12:42:16 2017 +0200

    Guard against schemas without QueryType (#547)

commit 1e3bb14
Author: Mikhail Novikov <[email protected]>
Date:   Tue Dec 19 14:06:13 2017 +0200

    Check null resolvers

commit 2e2c9b1
Author: Mikhail Novikov <[email protected]>
Date:   Mon Dec 11 16:09:06 2017 +0200

    Simplify API
@JakeDawkins
Copy link
Contributor

JakeDawkins commented Jul 13, 2018

We should see if we can help people transition to the new syntax. How can we detect if something was a comment vs if something is a description?

@orta
Copy link
Contributor

orta commented Nov 8, 2018

Controversial opinion alert 🔥

What about if graphql-tools keeps the ability to use # for descriptions as an option? I rarely want to write comments on a schema, but always want to add annotations to the types.

E.g. I'd prefer to have lot pages and pages of this:

screen shot 2018-11-08 at 4 27 21 pm

in comparison to pages of this:

screen shot 2018-11-08 at 4 26 11 pm

I know it's maintenance for you, but I think a lot of people would be happy setting an option to keep that deprecated style around

@yaacovCR
Copy link
Collaborator

I think we can close this. We will track upstream graphql-js and deprecate when they do, preserving flag as long as possible. Upgrade path will be for users to search their schemas for old style comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Focuses on documentation changes easy enhancement help wanted Extra attention is needed
Projects
None yet
5 participants