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

Error when used with eslint-plugin-graphql in v2.5.0 #81

Closed
eldh opened this issue Jan 19, 2018 · 12 comments · Fixed by #111
Closed

Error when used with eslint-plugin-graphql in v2.5.0 #81

eldh opened this issue Jan 19, 2018 · 12 comments · Fixed by #111

Comments

@eldh
Copy link

eldh commented Jan 19, 2018

What version of eslint are you using?
4.15.0

What version of prettier are you using?
1.10.2

What version of eslint-plugin-prettier are you using?
2.5.0

Please paste any applicable config files that you're using (e.g. .prettierrc or .eslintrc files)
Relevant rules:

{
    'prettier/prettier': [
      1,
      {
        printWidth: 120,
        trailingComma: 'es5',
        singleQuote: true,
        semi: false,
      },
    ],
    'graphql/template-strings': [
      2,
      {
        env: 'literal',
        schemaJson: require('./graphql/schema.json'),
      },
    ],
}

What source code are you linting?
This seems to happen for any .graphql file while using eslint-plugin-graphql (I'm on version 1.4.1).

What did you expect to happen?
It should lint correctly.

What actually happened?
In eslint-plugin-prettier 2.4.0 everything works, but since 2.5.0 I get the following error:

Syntax Error: Unexpected Name "ESLintPluginGraphQLFile" (1:1)
> 1 | ESLintPluginGraphQLFile`#import "./redacted-file-name.graphql"
    | ^

Assuming this has to do with the plugin now passing the filename to prettier, and eslint-plugin-graphql causes graphql files to have a weird filename reported somehow?

@not-an-aardvark
Copy link
Member

Could you provide the contents of your graphql file? (If the file is confidential, you could also provide a different file that causes the same error.)

@eldh
Copy link
Author

eldh commented Feb 13, 2018

It's the same with any file. Example:

query TaskCount($organizationShortName: ID!, $filter: String!, $sort: [String], $skip: Int) {
  organization: organizationByShortName(shortName: $organizationShortName) {
    id
    tasks(filter: $filter, sort: $sort, skip: $skip) {
      totalCount
    }
  }
}

@not-an-aardvark
Copy link
Member

It seems like this is happening because eslint-plugin-graphql uses a processor on a graphql file to allow it to be read as JavaScript, so the text received by eslint-plugin-prettier is JavaScript rather than graphql. However, since eslint-plugin-prettier is passing the .graphql file extension to prettier, prettier is trying to parse the JavaScript as GraphQL, resulting in a parse error.

A similar problem would probably occur when using other processors (e.g. eslint-plugin-markdown), because prettier would get the embedded JavaScript and treat it as a markdown file due to the filename.

This makes me think that maybe it wasn't a good idea to pass the filename to prettier to change the parsing mode. Essentially, there are two types of filenames:

  1. JavaScript variants, where the can be parsed directly as JavaScript with some syntax extensions determined by the file extension (e.g. .jsx, .vue)
  2. File formats containing embedded JavaScript, where the JavaScript is extracted by a processor (e.g. .html, .md, .ejs, .graphql*)

Passing the filename to prettier works well for the first case, but not for the second case.

*GraphQL doesn't actually have embedded JavaScript, but it's handled by a processor in the same way.

@eldh
Copy link
Author

eldh commented Feb 17, 2018

Yeah, if that's the case it seems like there could be a number of similar problems with other file types. 👍

@amay
Copy link

amay commented Apr 8, 2018

@not-an-aardvark @eldh I'm running into this exact same problem and curious if there are any work arounds?

@not-an-aardvark
Copy link
Member

As a workaround, you could downgrade to eslint-plugin-prettier@2.4.0.

I'd like to solve this issue, but I'm not sure what the best solution would be. We could add a configuration option for users to avoid passing in the filename to prettier, but that doesn't seem very user-friendly (it would probably be pretty difficult to explain to users what the option does). Maybe it would be a good idea to add something to ESLint core so that rules can tell when a processor is being run, but that seems a bit leaky too.

@eldh
Copy link
Author

eldh commented Apr 8, 2018

Yeah I've pinned version to 2.4.0 for now.

@amay
Copy link

amay commented Apr 8, 2018

Good call. The work around was right there in the original issue description. Sorry for not reading more closely and thanks for the quick responses.

@mathieutu
Copy link

Hi, I've the same issue with the 5.6.2 version.
Is something planned about this?
Thanks.

@not-an-aardvark
Copy link
Member

I haven't figured out a better solution, so I think the best way forward might just be to add an option to eslint-plugin-prettier like processedFileExtensions: ['graphql', ...] that would prevent it from using prettier's filetype inference on files with the given extensions.

@hedgepigdaniel
Copy link

hedgepigdaniel commented Sep 9, 2018

I found that its also possible to work around this problem without downgrading eslint-plugin-prettier by instead using its configuration option to specify the parser:

rules: {
  'prettier/prettier': [1, {
    parser: 'babylon',
  }],
},

Not sure if this has any unintended side effects - I assume not, since you would not need to use anything except a javascript parser from within eslint (which only deals with javascript).

@BPScott
Copy link
Member

BPScott commented Sep 26, 2018

After doing some deep pondering trying to wrangle this. I believe I understand what is going on and the best way for fix this.

I don't think there needs to be a processedFileExtensions option. Instead people can disable the prettier rule for a subset of files using eslint overrides. Add an overrides section to the bottom of your eslint config, after your rules: {} like so:

{
  "rules": {},
  "overrides": [
    {
      "files": ["*.graphql", "*.gql"],
      "rules": {
        "prettier/prettier": "off"
      }
    }
  ]
}

This shall disable the prettier rules for graphql, instead of forcing the parser to be potentially incorrect for all files.


The problem occurs because eslint-plugin-graphql registers an eslint processor that runs over .graphql and .gql files which transforms those files into a block of JS that eslint can understand (basically wrapping the file content in a tagged template literal so it becomes valid JS). This processor runs for all files before all rules, and there is no way to say "only allow this parser for a subset of rules". This is reasonable as processors are designed to extract javascripty parts from a file rather than teach eslint about a whole new language and you'd want all existing rules to run over those pieces of extracted JS.

I don't think there is any value in forcing the parser for graphql files as the processor is not considered autofixable and the processor currently swallows up all linting issues that aren't their own rules. Thus issues raised by the prettier/prettier rule would never be visible and would never be autofixed. There is no point in running prettier over these files if the results will never be exposed. If eslint-plugin-graphql fixes their code to expose rule violations other than their own then it would be straightforward to enable prettier, but forcing the 'babylon' parser, for graphql and gql:

{
  "rules": {},
  "overrides": [
    {
      "files": ["*.graphql", "*.gql"],
      "rules": {
        "prettier/prettier": ["error", {"parser": "babylon"}]
      }
    }
  ]
}

not-an-aardvark pushed a commit that referenced this issue Sep 26, 2018
* Ignore files in .prettierignore

Fixes #88

* Force the babylon parser when parsing not-js files

Forcing the parser stops errors that are caused by trying to run a JS
fragment through the graphql / markdown parsers.

The 'html' parser has not yet been released yet, but will be in Prettier
1.15.

This uses a block list over an allow list because I expect the list of
"file types with a prettier parser that could contain javascript fragments"
will grow at a slower pace than "file types with a prettier parser that
are variations of the javascript language".

Fixes #98, Fixes #81
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants