yuki24 / artemis

Ruby GraphQL client on Rails that actually makes you more productive
MIT License
207 stars 14 forks source link

Add a rake task that checks GraphQL query/mutation types #56

Open yuki24 opened 5 years ago

yuki24 commented 5 years ago

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 5 years ago

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.

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

yuki24 commented 5 years ago

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

daemonsy commented 5 years ago

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 5 years ago

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 5 years ago

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 5 years ago

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]