yuki24/artemis

Add a rake task that checks GraphQL query/mutation types

Opened this issue · 6 comments

yuki24 commented

In a fast-paced project, it is common to change query/mutation types very frequently. It would be very helpful if there was a command that only checks types so developers will be notified of type errors early.

daemonsy commented

Hey @yuki24 we're using Artemis with good success, thank you. It was a good surprise to find out that it relies on graphql-client, I'm using it with an app where we used graphql-client.

I'd like to add a spec where we check if all operations are valid. Currently, it's expressed as

expect { MyClient.preload! }.not_to raise_error

This indirectly works because preload! throws error on parsing from the underlying Github Client.

However, it also produces a lot of constance redefinition errors.

  • A simple solution might be to make load_constant noop for a defined constant? I hope no one is relying on constant redefinition now.

  • Or introduce an API either in the library or test helper that parses all the documents, asserting that their valid. (For this case, prefer error messages over throwing errors?)

Any thoughts on this? I'm interested in taking it on.

yuki24 commented

@daemonsy what are you referring to by "constance redefinition errors"? Could you post examples here?

daemonsy commented

Hey @yuki24 sorry bad phrasing, they are warnings, not errors.

/Users/saw/.gem/ruby/2.3.6/gems/artemis-0.4.0/lib/artemis/client.rb:199: warning: already initialized constant GraphAPI::GetMember
/Users/saw/.gem/ruby/2.3.6/gems/artemis-0.4.0/lib/artemis/client.rb:199: warning: previous definition of GetMember was here
yuki24 commented

Interesting. At least in development Artemis should auto-load the constants on every request when a GraphQL file is updated. How are you running your test suite? Do you add the expectation to every spec or it's in its own spec?

daemonsy commented

It's in own spec, I think I've bumped into only a few times where I've might have been in a console or something.

  1. So it's definitely possible to call this method once and not trigger the warning if we're careful about it.
  2. Using it to test queries are valid is indirect, I see what I've done more like a hack.

Also see that in the Railtie this method will be called if config.eager_load is true, which makes perfect sense.

Currently thinking about two different concerns:

  1. Since we're on this topic, regardless of how the preload! method is called, should we make it a noop if the constant is already defined?

  2. What type of APIs do we have in mind for helping developers validate their queries?

yuki24 commented

The #preload! method, as the name implies, is not supposed to be called multiple times and I'm a bit hesitant to add workarounds for unintended cases. You should just use reload! on a normal rails console.

That said, I like your idea of calling MyClient.preload! to make sure everything is in a good shape. Perhaps defining a rake task that does it and adding it to the default task should work for most people:

namespace :graphql do
  task :validate do
    MyClient.preload!
  end
end

task default: ["graphql:validate", :your, "choise:of", :tasks, :spec]