Re-introduce support for descriptors

[jnwng]

#99 was reverted due to issues #104 and #106. @jamiter suspects this has to do with the inclusion of graphql/utilities/buildASTSchema to add descriptors, which i believe led to #106 and this particular line

additionally, we need to make sure our test suite is robust enough to handle browser-level testing to make sure we can't release changes that either 1) don't get bundled up properly or 2) are using browser-incompatible libraries (in this case, process)

its also possible that small changes upstream will allow us to use this on the client, but in the off-chance that this isn't easily changeable we may need to revert this feature too.

[malekjaroslav]

Some news on this?

[jamiter]

@jnwng, it looks like that line has been removed from graphql, so we should be able to revert the revert! What do you think?

See this file and especially this commit. As you can see it was already fixed with a typeof process !== 'undefined' (see this commit 2 months ago).

I checked the rest of the repo and no it doesn't seem to use process anywhere significant anymore.

A test suite for browser-level testing is still very welcome of course. Maybe someone with more knowledge on how to do that can give it a shot (in another ticket?).

https://docs.travis-ci.com/user/gui-and-headless-browsers/

[jnwng]

one spot of trouble we'll run into is that graphql-tag supports various ranges of graphql, so while this may work for the newest graphql, it won't be backwards-compatible for other versions — this might need a major version bump to force users to install only graphql@^0.12.0 and above.

[jamiter]

You're right. I checked and the fix from 2 months ago never got released until 0.12. To bad...

Are you ok with actually doing a major version bump?

[jamiter]

There might of course be the possibility to check the version and if it's 0.12 or higher add the description. But that feels kinda ugly.

[jamiter]

I just read (the recent added) release notes of 0.12. It now supports descriptors out of the box using strings:

"This is a type descriptor"
type Foo {
  "This is a field descriptor"
  bar: String

  """
  Multi line also works!
  Pretty awesome, huh?
  """
  baz: Int
}

graphql/graphql-js#927

This also made me realise that a description isn't just a string in the schema, but a type with a kind and a value. That's the reason it didn't work for everyone...

So in short: I think we should inform people about the new syntax and not add the comment descriptions. We get the new syntax for free anyway!

P.S. highlighting in GitHub and IDEs isn't ready for it yet, but it works anyway.

[jnwng]

im going to close this issue for now — i dont think that graphql-tag is where people should find out about support for new graphql syntax. thanks for digging in @jamiter!

[DevNebulae]

@jnwng It would maybe be useful if there was a note on the front-page README.md noting that the comment syntax changed in the recent update. What is your stance on this?

[jnwng]

typically i would want to delegate all information about the GraphQL syntax itself to the libraries that are handling the syntax itself. also, given that this isn't changed behavior in the graphql-tag library, i would hesitate to add too much information about it in this README.md

however, since it seems like enough people are interested in this, it wouldn't hurt to add a "Syntax Support" section that diverts to the graphql-js implementation as well as some notes on things like comments / descriptors. happy to accept a PR for the latter since im not exactly sure what information would be helpful to @DevNebulae and @jamiter to have seen. does that sound reasonable?

[DevNebulae]

@jnwng I think that the biggest discrepancy and confusion starts because Apollo, the biggest third-party GraphQL library provider so to say, still uses the old syntax. The confusion on my end started when I didn't see descriptions at first, but after I removed the gql tag it worked. That's when I made the link. So a little note on "Comment syntax" would be handy.