feat: add a check for deprecation errors

[koddsson]

Hey there! 👋

#14 describes the need for warnings on deprecated fields which is something I'd like as well so I fiddled around a bit and came up with this that seems to be working in my tests at least. Not sure if I need to make tests for appollo & graphql separately or handle any logic there separately. Also not sure if this should live somewhere else then where I put it. If there should be a rule to opt into or what so I'd appreciate any help on getting this PR up to standard 😄

TODO:

• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests pass
• Update CHANGELOG.md with your change
• If this was a change that affects the external API, update the README

[apollo-cla]

@koddsson: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[koddsson]

Those 4 tests were failing when I checked out the project FWIW

[jnwng]

@koddsson thanks for the PR! overall, things look good, and this is one of our oldest outstanding issues.

a couple of things that would be useful to add to this — like other rules in eslint, it would be nice to be able to configure how important these errors ultimately are. take a look at the required-fields and some other custom rules we have in place. this will allow us to ship this sooner rather than later without throwing errors on people's setups.

i think the test case you've provided is sufficient to test the functionality provided here.

when you have a chance, make sure to sign the CLA and fulfill the tasks laid out in the PR checklist. as for CI, im hoping its a transient error, but its an infrastructural problem, not a code problem. we'll get it sorted out as this PR progresses.

[jnwng]

can we add a rule to toggle the severity of these errors?

[jnwng]

@koddsson if you could rebase, #91 should have resolved some issues with the graphql-js dependency

[koddsson]

@jnwng Can you help me with how I put a guard in place to only report if the rule is enabled? I've been digging around and I'm not sure how I'd do that.

edit: nevermind, I think I got it figured out.

[koddsson]

I added this here to validate that the rule is not reporting when it's not enabled.

[koddsson]

I basically took the code from graphql/graphql-js@063148d instead of importing the function findDeprecatedUsages and calling that.

[koddsson]

@jnwng Could you take another peek at this if you have a spare moment 🙇

[jnwng]

woohoo! looks great. thank you for adding test cases and documentation :)

[jnwng]

@koddsson thank you so much for your hard work and patience. i'm just double checking with @stubailo about the code we're taking from graphql-js and once that's all resolved, i'll publish to npm

[jnwng]

@koddsson in retrospect, it feels a little simpler to import the findDeprecatedUsages helper from graphql-js itself. do you mind clarifying why you wanted to pull the code from there instead of using that helper?

[koddsson]

Hey @jnwng! Sorry I've been out for a bit.

do you mind clarifying why you wanted to pull the code from there instead of using that helper?

If memory serves me right, it's because findDeprecatedUsages takes in the whole AST and iterates through it and gives you back the errors, if any. Since we are already iterating and don't have access to the whole AST we can't use that function here and needed to copy paste from the original.

[nodkz]

@jnwng @stubailo any progress with publishing this merged changes to npm?

[jnwng]

I’m so sorry! I thought I had published this already. Just one quick change to make sure we’re pointing to source and I’ll be happy to publish this. Thanks for your patience!

…

On Mon, Jan 8, 2018 at 6:35 AM Pavel Chertorogov ***@***.***> wrote: @jnwng <https://github.com/jnwng> @stubailo <https://github.com/stubailo> any progress with publishing this merged changes to npm? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABERRrAf2pfa1LdOUuvJzKQpbhABRoeJks5tIie2gaJpZM4QXyHt> .

[koddsson]

Hey @jnwng! Is there something I can help with to get this published? I'm not sure what "Just one quick change to make sure we’re pointing to source" is or if that's something I can help with but feel free to ping me if I can help in any way <3

[jnwng]

@koddsson i wanted to add attribution for the code that was moved here from graphql-js, as well as address #100 (the code here doesn't take into account enum values). the problem with #100 right now is that its failing in older versions of node, so i'm just trying to root down the problem there before releasing this. once #100 is resolved we can publish.

[koddsson]

Thanks for the update @jnwng!

[jnwng]

@koddsson okay, took me a bit to figure out what was going on, but this should be working and released as of eslint-plugin-graphql@1.5.0. let me know if you have any problems!