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

Prettier assumes untagged template string parameter is graphql #7790

Open
tdjone1 opened this issue Mar 18, 2020 · 14 comments
Open

Prettier assumes untagged template string parameter is graphql #7790

tdjone1 opened this issue Mar 18, 2020 · 14 comments
Labels
area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:graphql Issues affecting GraphQL lang:javascript Issues affecting JS

Comments

@tdjone1
Copy link

tdjone1 commented Mar 18, 2020

Prettier 1.19.1
Playground link

--parser typescript

Input:

function graphql(notgql: string): string {
  return notgql + ' something'
}

const x = graphql(`{id}`)

// following code will cause prettier error: Syntax Error: Unexpected Name "x" (1:1)
// const y = graphql(`x {id}`)

Output:

function graphql(notgql: string): string {
  return notgql + " something";
}

const x = graphql(
  `
    {
      id
    }
  `
);

// following code will cause prettier error: Syntax Error: Unexpected Name "x" (1:1)
// const y = graphql(`x {id}`)

Expected behavior:

The template strings passed to graphql() should not be reformatted because there is no guarantee that they are graphql queries.

@alexander-akait
Copy link
Member

Do not use graphql name, it is special name and have special formatting, duplicate of #5588

@tdjone1
Copy link
Author

tdjone1 commented Mar 18, 2020

When a "formatter" starts dictating what names can be used for functions it has gone beyond the scope of formatting.

@thorn0
Copy link
Member

thorn0 commented Mar 18, 2020

Yep, that's a known issue: #5588.

@tdjone1
Copy link
Author

tdjone1 commented Mar 18, 2020

Not sure this really qualifies as a duplicate of #5588. That issue is about tagged templates.

@thorn0
Copy link
Member

thorn0 commented Mar 18, 2020

Good point, but essentially it's the same problem.

@fisker
Copy link
Member

fisker commented Mar 20, 2020

We should reopen this. This is not about TaggedTemplateExpression and we don't have similar logic for other languages.

This comment

/*
* react-relay and graphql-tag
* graphql`...`
* graphql.experimental`...`
* gql`...`
*
* This intentionally excludes Relay Classic tags, as Prettier does not
* support Relay Classic formatting.
*/
didn't mention graphql(), we should just exclude CallExpression
(parent.type === "CallExpression" &&
parent.callee.type === "Identifier" &&
parent.callee.name === "graphql")))


html: only check TaggedTemplateExpression

node.type === "TaggedTemplateExpression" &&

markdown: same

parentParent.type === "TaggedTemplateExpression" &&

css: same

parent.type === "TaggedTemplateExpression" &&

@thorn0
Copy link
Member

thorn0 commented Mar 20, 2020

It's enough to mention this in #5588. No need to treat this as a separate issue.

@fisker
Copy link
Member

fisker commented Mar 20, 2020

But we are not going to fix #5588 in the near future, right?

But graphql() should consider a bug, and it's easy to fix.

@alexander-akait
Copy link
Member

Okay, let's mark it as bug until we do not disable prettify tagged templated

@alexander-akait alexander-akait added area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:graphql Issues affecting GraphQL type:bug Issues identifying ugly output, or a defect in the program labels Mar 20, 2020
@thorn0
Copy link
Member

thorn0 commented Mar 20, 2020

Okay, but calling it a bug is strange, given that it was intentionally implemented in #2781 as a solution for #2780. Conceptually, it's the same feature as formatting tagged templates. As we don't know how many users rely on this currently, we can't just remove it, which means it needs configurability. So it's really the same problem as #5588 / #6626.

@fisker
Copy link
Member

fisker commented Mar 20, 2020

I didn't know #2781 , if we don't want remove this feature now, maybe we can make it only work on second argument?

@alexander-akait
Copy link
Member

@thorn0 I agree with you, but we still haven’t decided how to fix it:

  1. Do not touch tagged templates and remove code respected for that behavior
  2. Allow to user setup tagged templates (if yes we should not close it)
  3. Maybe other ideas?

Sometimes I found formatting tagged templates is very useful, I would not want to delete it, but I understand that this is a potentially dangerous action, with limitations, maybe we should describe them in docs

@thorn0
Copy link
Member

thorn0 commented Mar 20, 2020

@fisker Yes, why not, sounds good.

@evilebottnawi Something like #6626 has to be implemented. I see no other ways.

@alexander-akait
Copy link
Member

@thorn0 Yep, but i think it is not high priority right now

@thorn0 thorn0 removed the type:bug Issues identifying ugly output, or a defect in the program label May 12, 2020
@thorn0 thorn0 added the lang:javascript Issues affecting JS label Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:multiparser Issues with printing one language inside another, like CSS-in-JS lang:graphql Issues affecting GraphQL lang:javascript Issues affecting JS
Projects
None yet
Development

No branches or pull requests

4 participants