Graphql-js: Schema printer should not cut description strings, because they are markdown

[nsf]

According to GraphQL spec, description strings contain markdown (https://graphql.github.io/graphql-spec/June2018/#sec-Descriptions). Amongst other features, markdown may contain code blocks (https://spec.commonmark.org/0.29/#fenced-code-blocks). It appears to me, that it's very incorrect to break description strings at arbitrary points as current printer does it: https://github.com/graphql/graphql-js/blob/48ea2d3e8df5e4fbbdb5e0ce67c0a3c219b024f8/src/utilities/schemaPrinter.js#L331
https://github.com/graphql/graphql-js/blob/48ea2d3e8df5e4fbbdb5e0ce67c0a3c219b024f8/src/utilities/schemaPrinter.js#L357-L367
Because, for example, it can make python code invalid (it's whitespace sensitive) or break a JSON string into multiple lines (JSON strings are not multilne literals).

[IvanGoncharov]

@nsf Thanks for reporting 👍
It's definitely an issue on our side but I'm not sure how to detect correct points to break descriptions.
Currently, we are in the feature freeze due to conversion to TypeScript so can we postpone fix for couple of weeks?

[nsf]

No need to rush it, in my view the correct way to do it is to not break description strings at all. Markdown alone without code blocks might also be sensitive to that iirc.

[IvanGoncharov]

@nsf I think you right about not breaking Markdown at all.
But we have use cases where schemas are autogenerated from some other sources and descriptions are formatted as one ridiculously long string.

What do you think about middle ground where we insert break only in descriptions that are more than 120 characters and don't have any new lines inside?

[mike-marcacci]

Could we internally defer description rules to an existing tool that understands markdown semantics? Perhaps prettier, or something less opinionated but with similar understanding of the language?

[nsf]

You can make it an option. There is already one related to desc strings in API (toggles them to out-dated comments form). As for default behaviour, I have no opinion on it. Works for me as long as there is a way to avoid breaking lines when printing schema.

[IvanGoncharov]

Could we internally defer description rules to an existing tool that understands markdown semantics?

@mike-marcacci Interesting idea.
However graphql-js is a reference implementation so we can't use stuff that's not available in other languages. At the moment we have only one dependency on iterall(just to support IE11) so it supper easy to port code from graphql-js to other languages.

Also, printSchema is frequently used in dev tools and many of them are browser-based so I don't think it's justified to increase bundle size just to break long strings.

There is already one related to desc strings in API (toggles them to out-dated comments form)

@nsf We plan to delete it in 16.0.0.

---

Thinking more about it, by adding new lines we actually change AST so parse(sdl) !== parse(printSchema(buildSchema(sdl))) which is problematic by itself even without breaking Markdown.

Also, Prettier sims to keep description intact for the same reason: Prettier Plaground
Moreover, prettier has the exact same policy for the long string and comments in JS so it's not GraphQL specific behavior.

I don't think we should be less opinionated that Prettier so I propose to just remove line breaks functionality completely. I don't think it's a big deal since most of IDE, terminals, online tools have word wrap enabled.
It will also solve the problem with even small changes in descriptions producing unintentionally huge diffs when you compare before & after SDLs.

@nsf @mike-marcacci What do you think?

[nsf]

Works for me as long as there is a way to avoid breaking lines when printing schema.

:+1:

[mike-marcacci]

@IvanGoncharov - avoiding an increase of the package size makes perfect sense. I'm perfectly fine with preserving the description completely, including long lines!