feat: add prettier! #2508
feat: add prettier! #2508
Conversation
🦋 Changeset detectedLatest commit: 4b4b659 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
@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 |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
The latest changes of this PR are available as canary in npm (based on the declared |
|
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. |
|
Total sidenote: I started hacking on my own GraphQL implementation in JS (more modular compared to the all-in-one This is far from being useable (implemented traversal but no printing yet), just something I wanted to drop here. |
|
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! |
|
we do have an issue to support |
There was a problem hiding this comment.
yes it for sure will be harder to work with new language features to get prettier support. But in reality I don't think stable graphiql should be running an experimentgraphql-js release so I think it is fine to merge this. So all those experimental features should be on some experimental graphiql release.
This comes back to the question me and @n1ru4l raised on how we can get graphql-js to easily support multiple RFC stage features
| import('prettier/standalone').then(({ default: prettier }) => { | ||
| import('prettier/parser-graphql').then(({ default: graphqlPlugin }) => { |
There was a problem hiding this comment.
I guess you can use Promise.all in this case, and async/await.
| import('prettier/standalone').then(({ default: prettier }) => { | |
| import('prettier/parser-graphql').then(({ default: graphqlPlugin }) => { | |
| const [{ default: prettier }, { default: graphqlPlugin }] = await Promise.all([ | |
| import('prettier/standalone'), | |
| import('prettier/parser-graphql'), | |
| ]); |
There was a problem hiding this comment.
I would need to make the parent function async as well, I can try this
|
@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? |
|
|
@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! |
|
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 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 either is fine, we can just start over if that makes more sense to you! It would also be tremendous to add this to |
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
formatfor the first time. I would prefer to use pre-fetching, but I'm not sure how to implementimport()prefetching in a cross-bundler compliant way. We could also manually prefetch on a triggered event, such as blur/focus of the format button.