Skip to content

Conversation

adek05
Copy link
Contributor

@adek05 adek05 commented Mar 29, 2019

Fragment variables are an experimental extension to GraphQL schema.
Formatting should follow the same rules as formatting an OperationDefinition node,

hence code is the same.

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Fragment variables are an experimental extension to GraphQL schema.
Formatting should follow the same rules as formatting an OperationDefinition node,
hence code is the same.
@lydell
Copy link
Member

lydell commented Mar 30, 2019

Do you think it would be bad to enable this by default and removing the option?

@adek05
Copy link
Contributor Author

adek05 commented Mar 30, 2019

Not really, gql parser should be robust enough to not cause issues. If this feature gets deleted in the future we will pick it up and fix when upgrading graphql-js dependency.

It is strictly a superset of existing language so should work fine. Not sure what is the sentiment about using something that is experimental.

@j-f1
Copy link
Member

j-f1 commented Mar 30, 2019

In the JS parser, we support (I believe) all the syntax proposals Babel does, so I think we should do the same here.

@j-f1
Copy link
Member

j-f1 commented Mar 31, 2019

Can you add a test with multiple fragment variables so the variable section is forced to break onto multiple lines?

@j-f1
Copy link
Member

j-f1 commented Mar 31, 2019

Sorry to not have asked for all these things at once. How are the fragment variables specified when using the fragment? Does there need to be printing support for that too?

@adek05
Copy link
Contributor Author

adek05 commented Mar 31, 2019

They are now defined on the root operation definition through a query transformation at build time. You don’t set them at fragment spreads as of yet, so no extra work is necessary at this point

@duailibe
Copy link
Collaborator

duailibe commented Apr 1, 2019

In the JS parser, we support (I believe) all the syntax proposals Babel does, so I think we should do the same here.

The problem is if any of those proposals get deleted in the future (or maybe they stop working eventually). Prettier will either have to continue to support those (unfortunate) or stop formatting user's code (which is unfortunate for users). We already carry the burden of having to support both of decorator proposals.

For this PR, I think we should do it and, if this doesn't get added to the language, let's hope the parser continues to support it for a while

@j-f1
Copy link
Member

j-f1 commented Apr 1, 2019

Should we have a policy that semver doesn’t apply to stage-x JS features?

@duailibe
Copy link
Collaborator

duailibe commented Apr 2, 2019

@j-f1 What?

@vjeux
Copy link
Contributor

vjeux commented Apr 2, 2019

@duailibe we need this for our GraphQL usage at Facebook. Would it be possible to get it merged? Thanks!

@j-f1
Copy link
Member

j-f1 commented Apr 3, 2019

@duailibe referring to your previous comment above about supporting experimental features. I was wondering if we should have a policy that we’ll stay up to date with breaking changes to proposals for JS in minor versions.

@j-f1 j-f1 added the priority:facebook blocker Issues that block Facebook from upgrading Prettier. (Facebook is a major Prettier supporter) label Apr 3, 2019
@duailibe duailibe merged commit 7c9823f into prettier:master Apr 4, 2019
@duailibe
Copy link
Collaborator

duailibe commented Apr 4, 2019

@adek05 Thanks!

@vjeux I'll cut a release today or tomorrow

@vjeux
Copy link
Contributor

vjeux commented Apr 4, 2019

Thanks!

@ikatyang ikatyang added this to the 1.17 milestone Apr 12, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. priority:facebook blocker Issues that block Facebook from upgrading Prettier. (Facebook is a major Prettier supporter)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants