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

Added support for the graph ql comment tag #4395

Merged
merged 4 commits into from May 9, 2018

Conversation

Projects
None yet
7 participants
@tjallingt
Copy link
Contributor

commented Apr 28, 2018

As described in the issue #4360
Prettier still adds a space in between the comment tag and the template literal.

Added support for the graph ql comment tag
As described in the issue #4360
Prettier still adds a space inbetween the comment tag and the template literal.
@j-f1

This comment has been minimized.

Copy link
Member

commented on src/language-js/embed.js in 7707a30 Apr 28, 2018

comment.value.trim() === "GraphQL"?

This comment has been minimized.

Copy link
Contributor Author

replied Apr 28, 2018

Yea i was doubting whether or not to trim the comment value...
My initial thought was to trim it but after considering it for a bit i figured it'd be better to be exact. Additionally the various other implementations of this comment tag also require it to specifically be /* GraphQL */:

https://github.com/michaelgmcd/vscode-language-babel/blob/master/grammars/Babel-Language.json#L1818

https://github.com/gandm/language-babel/blob/master/grammars/Babel%20Language.json#L1111

kumarharsh/graphql-for-vscode@91917f0

I'm willing to change this if desired but I figured I'd just let you know why I chose to not use trim 👍

Let me know what you think

This comment has been minimized.

Copy link
Member

replied Apr 29, 2018

Sounds good 👍

This comment has been minimized.

Copy link
Collaborator

replied Apr 29, 2018

Might be worth embedding this information as a comment.

This comment has been minimized.

Copy link
Contributor Author

replied Apr 29, 2018

Great suggestion. I figured adding the exact links would be a bit superfluous so i just mentioned that "other implementations" also expect that exact value. Hope it is clear enough 👍

@duailibe

This comment has been minimized.

Copy link
Member

commented Apr 28, 2018

After reading #4395 (comment) I don't think we should do this. Feels really hack-ish. The suggestion in #4360 (comment) seems like a better solution

const gql = String.raw;

gql`query MyQuery {}`;
@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2018

@duailibe well I mean this is the way those other tools do this as well, so its not more or less hacky than those other tools ("would you jump off a bridge if your friends did" fallacy i know...) additionally

const gql = String.raw;

gql`query MyQuery {}`;

also feels kinda hacky...

but its your call, I understand your hesitation to merge this 😕

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented on src/language-js/embed.js in 7707a30 Apr 28, 2018

maybe this should not be some but instead use the last comment from this array...

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2018

Actually after thinking about it, the String.raw workaround doesn't work if you are using the actual gql tag in the same file. (Which i guess you could fix by using one of the other template tags prettier formats as graph ql but thats a workaround ontop of a workaround...)

Additionally I dislike it as a workaround because of someone sees gql`...` but doesn't see the const gql = String.raw part they might get a completely wrong idea of what the code is doing only because prettier doesnt support formatting arbitraty template literals as graph ql...

@jaydenseric

This comment has been minimized.

Copy link

commented Apr 29, 2018

@suchipi

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@duailibe could you expound on why you think we shouldn't do this?

@duailibe

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@suchipi Just because we already have a pretty consistent way to format non-JS stuff in JS, that is using template literals or <style> JSX tags and it's been working really well.

Comments is an area of Prettier that works good enough for the effort we've put in but we still get reports of it's behavior so it seems a fragile strategy to rely on them for this kind of stuff.

That said, I changed my mind and I don't oppose to it. If other maintainers are comfortable with adding this functionality, I'm on board.

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

So what is the status of this discussion?
Are you willing to merge this provided I fix the failing test?

@j-f1

This comment has been minimized.

Copy link
Member

commented May 4, 2018

I’m 👍 on the change, and @duailibe seems have changed their mind and is also 👍 (or at least not 👎).

@duailibe

This comment has been minimized.

Copy link
Member

commented May 4, 2018

@tjallingt to fix the test, you probably need to skip the check of the AST here: https://github.com/prettier/prettier/blob/master/src/language-js/clean.js

More specifically, something along these lines:

// styled-components, graphql, markdown
if (
ast.type === "TaggedTemplateExpression" &&
(ast.tag.type === "MemberExpression" ||
(ast.tag.type === "Identifier" &&
(ast.tag.name === "gql" ||
ast.tag.name === "graphql" ||
ast.tag.name === "css" ||
ast.tag.name === "md" ||
ast.tag.name === "markdown")) ||
ast.tag.type === "CallExpression")
) {
newObj.quasi.quasis.forEach(quasi => delete quasi.value);
}
if (
ast.type === "TemplateLiteral" &&
parent.type === "CallExpression" &&
parent.callee.name === "graphql"
) {
newObj.quasis.forEach(quasi => delete quasi.value);
}
}

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

That should fix the failing test 👍

I really dislike that I had to duplicate the hasGraphQLComment stuff so maybe that should be exported to clean.js? (don't know what your stance on this is)

EDIT: this also still adds an undesired space between the comment tag and the template literal, I could take a look at fixing that if desired but I have a hunch that might be kinda tricky 😕

@suchipi

This comment has been minimized.

Copy link
Member

commented May 9, 2018

@tjallingt let's not worry about the space between the comment and the template literal tag for now. Also, I'm not sure we have a consistent approach with duplicated logic in clean; do whatever you feel is best.

@suchipi suchipi added this to the 1.13 milestone May 9, 2018

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

The it's ready for review and merge I think

@lydell

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2018

@tjallingt There are some merge conflicts to fix before merging, though

@suchipi

This comment has been minimized.

Copy link
Member

commented May 9, 2018

Fixed the conflicts

@suchipi

suchipi approved these changes May 9, 2018

@tjallingt

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

oh mb didn't see them 😕
Thanks for fixing @suchipi 👍

@suchipi suchipi merged commit 8cf5914 into prettier:master May 9, 2018

4 checks passed

codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 96.41% (+<.01%) compared to ab452dd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@lipis lipis removed this from the 1.13 milestone May 9, 2018

@lipis lipis added this to the 1.13 milestone May 9, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.