apollographql / eslint-plugin-graphql

:vertical_traffic_light: Check your GraphQL query strings against a schema.
https://www.npmjs.com/package/eslint-plugin-graphql
1.21k stars 103 forks source link

Should register props as being used when combined with eslint-react #27

Open swernerx opened 7 years ago

swernerx commented 7 years ago

When writing such code:

class ElectivesView...

const withElectivesData = graphql(ElectivesQuery, {
  options: (ownProps) => ({
    variables: ownProps.electiveVariables
  })
})

export default connect(mapStateToProps, mapDispatchToProps)(withElectivesData(ElectivesView))

I can the following errors:

Generally the plugin works fine, but I think like the JSX plugin registers tags which are used to the imported symbol/variable counter (to find unused variables/imports) we have to do something about the GraphQL methods so that these are correctly regarded their usage of our defined props.

stubailo commented 7 years ago

Interesting - I bet react-redux has the same issue, can you find where they have solved it?

DianaSuvorova commented 7 years ago

That's a good idea. I don't think this library can do anything about it. It is a question for eslint-plugin-react. As a matter of fact there is already a similar issue created: https://github.com/yannickcr/eslint-plugin-react/issues/1097.

Generally it would be nice if eslint-plugin-react would be able to coordinate with this plugin (and some other react-related plugins like flow). Let me look into that 🙂

stubailo commented 7 years ago

Thanks let me know what you find!

DianaSuvorova commented 7 years ago

@stubailo Here is what I think we can do. Eslint provides context.markVariableAsUsed interface. For example this flow plugin uses it to mark flow types as used for 'no-unused-vars' rule.

I think we could do the same. Graphql plugin would need to markVariableAsUsed for all props that are used by graphql. While react plugin would need to be modified to read this list.

There are few questions I am not sure about:

I am planning to set up a test repo with a proposed changes for both plugin libraries and see if it works....

-Diana

DianaSuvorova commented 7 years ago

Ok, nevermind me. It was way less exciting than I thought 🙂

I created a PR to verify the graphql use case. It is supported by eslint-plugin-react correctly. I think there is a misunderstanding at what component level props needs to be configured. This was the case for a example I referenced above at least.

As for the issue reported in this thread - I would need a whole code snippet to repro. And it is for sure not this plugin's issue. So should be closed here.

@swernerx , please see the discussion here. If you still have an issue please report it there.

Thanks, Diana