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

Make print() break arguments over multiple lines #2797

Merged
merged 9 commits into from
Sep 21, 2020
Merged

Make print() break arguments over multiple lines #2797

merged 9 commits into from
Sep 21, 2020

Conversation

draperunner
Copy link
Contributor

Hi! We are using the print function to pretty-print our queries. It works quite well, but there is one thing that I think could be improved upon, and that's the rendering of arguments.

This first image illustrates our problem. The main query takes a lot of parameters, and when formatting the query, all of these are put on the same line.

image

This next image illustrates the result of the changes made in this PR. All parameters are put on individual lines, and object values are also "prettified".

image

Questions

  1. Should a parameter be kept on one line if there is only the one?

image

  1. Should perhaps array values also be broken over multiple lines, or is the following okay?

image

@IvanGoncharov
Copy link
Member

@draperunner Thanks for PR 👍
I think we need to follow prettier lead on how to format GraphQL queries.
Can you please research how prettier format arguments?

@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Sep 13, 2020
@IvanGoncharov
Copy link
Member

Should perhaps array values also be broken over multiple lines, or is the following okay?

Let's first solve arguments and latter move to other areas in separate PRs.
The long-term plan is to mirror a prettier approach to formatting but it should be done as a series of PRs.

@draperunner
Copy link
Contributor Author

draperunner commented Sep 15, 2020

Okay, I've tested how Prettier handles arguments. See examples below.

  • Arguments are put on multiple lines if line length exceeds configured tabWidth (80 chars is default)
  • Objects are prettified only if the line length exceeds configured tabWidth (80 chars is default)

So, what is your thoughts going forward with this? The existing printer in this projects has no awareness of line length, which is important to Prettier. Implementing it would be like reimplementing Prettier. Maybe the graphql part of Prettier could be a dependency of graphql-js? Or maybe one shouldn't use grahpql-js for formatting, but Prettier instead? Or, one could assume that line length is always exceeded, and always break the arguments over multiple lines.

{
  trip(numTripPatterns: 3) {
    tripPatterns {
      distance
    }
  }
}
{
  trip(numTripPatterns: 3, wheelchairAccessible: false) {
    tripPatterns {
      distance
    }
  }
}
{
  trip(
    numTripPatterns: 3
    wheelchairAccessible: false
    maxPreTransitWalkDistance: 2000
  ) {
    tripPatterns {
      distance
    }
  }
}
{
  trip(to: { place: "SOME_ID", coordinates: { latitude: 0, longitude: 0 } }) {
    tripPatterns {
      distance
    }
  }
}
{
  trip(
    to: { place: "SOME_ID", coordinates: { latitude: 0, longitude: 9 } }
    from: { place: "OTHER_ID", coordinates: { latitude: 0, longitude: 9 } }
  ) {
    tripPatterns {
      distance
    }
  }
}
{
  trip(
    to: {
      place: "SOME_ID"
      coordinates: { latitude: 0, longitude: 9 }
      description: "Lorem ipsum is a long text"
    }
    from: { place: "OTHER_ID", coordinates: { latitude: 0, longitude: 9 } }
  ) {
    tripPatterns {
      distance
    }
  }
}

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Sep 15, 2020

@draperunner Prettier is an unnecessary big dependency for projects that only work with GraphQL.
Also, the main purpose of graphql-js is to be a reference implementation for other languages and they should be able to use our implementation as a reference on how to implement pretty printing.
Another factor is that we working on adding support for Deno so we can't depend on other NPM packages.
Can't speak for the prettier team but in the long run, it would be great if prettier use graphql-js for formating.

If you want to work on this please focus this PR on arguments, and create separate PRs for object fields.
Also please locate and copy relevant test-cases from the Prettier codebase but please change them to match the style of our existing tests.
As for line width, you can create a constant equal to 80 and for now, use it only with arguments.

Proper logic for this is to be implemented, therefore are the tests failing now
Prettier does not use trailing commas if args go over multiple lines.
The 'prints kitchen sink' test is updated to reflect desired behavior
for multiline arguments, but where objects don't get prettified.
MAX_LINE_LENGTH is a hard-coded constant set to 80 chars for now.
@draperunner
Copy link
Contributor Author

@IvanGoncharov I have now added a couple of tests. I have added the MAX_LINE_LENGTH constant and arguments are only put on multiple lines if this length is exceeded.

I've updated the prints kitchen sink test by copying the results from running Prettier on __fixtures__/kitchen-sink.graphql, but I've removed the parts that are not relevant now: some object prettifying and other formatting that Prettier did.

Another thing I noticed is that Prettier does not use trailing commas when arguments are on multiple lines, so I have removed that to match Prettier's behavior.

@IvanGoncharov IvanGoncharov merged commit b9cf5f3 into graphql:master Sep 21, 2020
@draperunner draperunner deleted the print-arguments-multiline branch September 21, 2020 12:43
tgriesser added a commit to tgriesser/graphql-js that referenced this pull request Nov 13, 2020
…ription

* master: (211 commits)
  Update deps (graphql#2844)
  resources: use named groups in RegExp (graphql#2840)
  TS: exclude integration tests from root tsconfig.json (graphql#2838)
  Flow: remove support for measuring Flow coverage (graphql#2837)
  CI: test on node 15 (graphql#2836)
  Update deps (graphql#2835)
  build: add support for experimental releases (graphql#2831)
  15.4.0
  Update deps (graphql#2827)
  Update deps (graphql#2825)
  integrationTests: add Flow test (graphql#2819)
  fix: ensure variance of types matches how values are used (graphql#2786)
  Cleanup '__fixtures__' leftovers (graphql#2818)
  Convert fixtures to be JS files (graphql#2816)
  Update deps (graphql#2815)
  benchmark: extract benchmarks into separate folder and run them on NPM package
  Update deps (graphql#2810)
  Update outdated documentation (graphql#2806)
  Make print() break arguments over multiple lines (graphql#2797)
  Added check for specific symbols in polyfills/symbols (graphql#2804)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants