graphiql icon indicating copy to clipboard operation
graphiql copied to clipboard

feat: add prettier!

Open acao opened this issue 3 years ago • 14 comments

This introduces prettier formatting by dynamically importing prettier's core library and the graphql plugin. This is how we've imported prettier in monaco-graphql's webworker compatible language service since 2020!

It's not ideal for UX, because the dependency is fetched only when using format for the first time. I would prefer to use pre-fetching, but I'm not sure how to implement import() prefetching in a cross-bundler compliant way. We could also manually prefetch on a triggered event, such as blur/focus of the format button.

acao avatar Jun 16 '22 08:06 acao

🦋 Changeset detected

Latest commit: 4b4b659d90666ebacb2a6ed4c6259287275dec40

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
graphiql Patch
@graphiql/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Jun 16 '22 08:06 changeset-bot[bot]

@benjie can you speak to this from the language spec perspective? is using prettier's formatting for graphql safe for us from a spec compliance perspective? graphql support is shipped with prettier core, and is a frequently maintained and updated capability as the language spec changes it would seem

acao avatar Jun 16 '22 08:06 acao

Codecov Report

Merging #2508 (4b4b659) into main (2d91916) will increase coverage by 2.65%. The diff coverage is 28.73%.

@@            Coverage Diff             @@
##             main    #2508      +/-   ##
==========================================
+ Coverage   65.70%   68.36%   +2.65%     
==========================================
  Files          85       71      -14     
  Lines        5106     4182     -924     
  Branches     1631     1376     -255     
==========================================
- Hits         3355     2859     -496     
+ Misses       1747     1318     -429     
- Partials        4        5       +1     
Impacted Files Coverage Δ
packages/codemirror-graphql/src/lint.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts 47.05% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts 95.65% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts 0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/mode.ts 79.48% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts 100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts 0.00% <0.00%> (ø)
packages/graphiql-react/src/editor/tabs.ts 5.76% <5.76%> (ø)
...aphiql-toolkit/src/create-fetcher/createFetcher.ts 58.62% <50.00%> (-2.10%) :arrow_down:
...ackages/graphiql-toolkit/src/create-fetcher/lib.ts 56.45% <58.33%> (-3.13%) :arrow_down:
... and 97 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 915b4ff...4b4b659. Read the comment docs.

codecov[bot] avatar Jun 16 '22 08:06 codecov[bot]

The latest changes of this PR are available as canary in npm (based on the declared changesets):

[email protected]
@graphiql/[email protected]

github-actions[bot] avatar Jun 16 '22 08:06 github-actions[bot]

I cannot speak on behalf of prettier, and I certainly don't know what it means for experimental features of GraphQL. I would imagine that there wouldn't be much pushback from prettier for supporting newer syntax so long as it is officially supported by graphql-js; however I do not know who is currently doing that work.

benjie avatar Jun 16 '22 12:06 benjie

Total sidenote: I started hacking on my own GraphQL implementation in JS (more modular compared to the all-in-one graphql-js) which is storing comments on the AST nodes: https://github.com/thomasheyenbrock/graphql-modular/blob/main/packages/language/src/index.ts

This is far from being useable (implemented traversal but no printing yet), just something I wanted to drop here.

thomasheyenbrock avatar Jun 17 '22 14:06 thomasheyenbrock

Very cool @thomasheyenbrock! I highly suggest reaching out to @dotansimha at the guild, as they have a very similar modular library!

@benjie to answer your question, prettier uses graphql-js parse and visit directly, so it honors whichever version of graphql-js you have installed. It is part of the core prettier project, and is kept up to language spec more than even graphiql by multiple contributors and maintainers to the prettier project

there is only one config option that is honored as well

imho, it would be most ideal to use a method exported from the official graphql-js project, so perhaps @saihaj , @IvanGoncharov or others could help with that. Obviously it would be most ideal to keep everything in house with the graphql org, but I worry that this will add more scope and confusion as users expect the same behavior that prettier provides as the dominant autoformatting tool in use for graphql

i don’t know how the prettier project feels about it, but perhaps they would like us to maintain it?

either way, prettier is being used to format the language spec now, so we should make sure that whatever we decide to use officially is consistent.

otherwise, people will copy paste queries from graphiql to their editor where prettier is enabled, and often need to apply more autoformatting because of the different output, which is one of the main reasons people have requested prettier formatting in graphiql. This was the reasoning for my choice to use prettier with monaco-graphql.

so whatever we decide to do, as long as it has parity with prettier‘s formatting output, it will make users happy!

acao avatar Jun 17 '22 16:06 acao

we do have an issue to support prettyPrint so it is prettier like formatting https://github.com/graphql/graphql-js/issues/3230

saihaj avatar Jun 18 '22 23:06 saihaj

@saihaj @thomasheyenbrock what if we „incubate“ this formatting utility in graphiql as an internal detail, and then propose it as a PR to graphql-js?

acao avatar Jun 24 '22 11:06 acao

@saihaj @thomasheyenbrock what if we „incubate“ this formatting utility in graphiql as an internal detail, and then propose it as a PR to graphql-js?

graphql-js wouldn’t take this since it would require prettier as a dependency

saihaj avatar Jun 24 '22 11:06 saihaj

@saihaj apologies for the confusion- we are proposing writing a custom formatter function that we maintain internally and/or contribute to graphql-js. Prettier would not be a dependency. See @thomasheyenbrock ‘s example above!

acao avatar Jun 24 '22 11:06 acao

@saihaj apologies for the confusion- we are proposing writing a custom formatter function that we maintain internally and/or contribute to graphql-js. Prettier would not be a dependency. See @thomasheyenbrock ‘s example above!

Ohhhh yes that would work!

saihaj avatar Jun 24 '22 13:06 saihaj

That sounds like a good proposal to me @acao! Just a custom printer that preserves comments would already be a huge improvement for GraphiQL, and we could iteratively see how to maybe incorporate that into graphql-js 👍

Does that mean we abandon this PR in favor of a custom printer, or would we still want to add prettier as a first step and do the custom printer as next iteration?

thomasheyenbrock avatar Jun 27 '22 08:06 thomasheyenbrock

@thomasheyenbrock either is fine, we can just start over if that makes more sense to you!

It would also be tremendous to add this to monaco-graphql, graphql-language-service-server and others!

acao avatar Jun 27 '22 20:06 acao