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

Release 1.10.0 #2100

Closed
14 tasks done
rmosolgo opened this issue Feb 11, 2019 · 10 comments
Closed
14 tasks done

Release 1.10.0 #2100

rmosolgo opened this issue Feb 11, 2019 · 10 comments
Milestone

Comments

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 11, 2019

Here are some goals I have for 1.10.0. Please feel free to contribute to the conversation by leaving a comment.

@rmosolgo rmosolgo added this to the 1.10.0 milestone Feb 11, 2019
@panthomakos
Copy link

Regarding pagination, the goals you have laid out in #1359 to support a clear and more explicit mapping for cursor based pagination is 👍. It took us a long while to decode and extend the existing relay pagination for our own purposes. The method names, purposes, and the integration with GraphQL were not very clear.

👍 to batching implementation. In particular, if (🤞) the concurrency abstraction is included in the next release, having a thread-safe batch implementation will be increasingly important. One more thing I would add here, existing implementations of loaders (like graphql-batch) effectively require an entirely new class to be created for each query or network call. This can become very cumbersome, so it would be great to provide a more lightweight way to have concurrent and batch loading lazy objects.

Regarding testing, we have found it difficult to unit test some things (like authorization) for somewhat nested objects because we need to write and execute full queries (instead of simply instantiating the graphql type with a context). Some kinds of helpers or easy ways to instantiate and unit test isolated graphql types would be great. Maybe this is already possible and we just need some examples 😀.

@michal-samluk
Copy link
Contributor

michal-samluk commented Feb 12, 2019

Regarding testing, we have found it difficult to unit test some things (like authorization) for somewhat nested objects because we need to write and execute full queries (instead of simply instantiating the graphql type with a context). Some kinds of helpers or easy ways to instantiate and unit test isolated graphql types would be great. Maybe this is already possible and we just need some examples 😀.

@panthomakos Not sure if this will help you, but you could implement node and/or nodes fields on Query and then fetch the object(s) you want to test. So then you skip building those big nested queries, but just fetch object(s) of specific type.

@panthomakos
Copy link

Not sure if this will help you, but you could implement node and/or nodes fields on Query and then fetch the object(s) you want to test. So then you skip building those big nested queries, but just fetch object(s) of specific type.

This is a good idea, and I think we will likely have top-level node objects in many APIs, but the downside is that testing forces you to create node objects even if you don't ever want to expose them. Additionally it makes testing in-memory only objects more difficult. By not having to write a query, I can wrap a simple struct or not-yet-persisted ActiveRecord object for speedier testing.

@Kenneth-KT
Copy link

Kenneth-KT commented Feb 13, 2019

Not sure if this will help you, but you could implement node and/or nodes fields on Query and then fetch the object(s) you want to test. So then you skip building those big nested queries, but just fetch object(s) of specific type.

This is a good idea, and I think we will likely have top-level node objects in many APIs, but the downside is that testing forces you to create node objects even if you don't ever want to expose them. Additionally it makes testing in-memory only objects more difficult. By not having to write a query, I can wrap a simple struct or not-yet-persisted ActiveRecord object for speedier testing.

Here is a method that does not need to expose a public node interface: we could construct a separate anonymous GraphQL schema in tests that subclasses existing schema and overwrites root query object with one for testing.

This is how it can be done:

describe TestTypeClass do
  let(:schema) do
    test_object = { name: 'testing123' }  # create/obtain your test object here
    Class.new(MySchemaClass) do  # subclassing your schema class
      query begin
        Class.new GraphQL::Schema::Object do
          graphql_name 'Query'
          field :testing, TestTypeClass, null: false
          define_method :testing do
            test_object
          end
        end
      end
    end
  end

  let(:result) do
    schema.execute(%|
      {
        testing {
          id
          name
          ...
        }
      }
    |).to_h
  end
  ...

Then we can start writing tests with the above setup:

describe 'field #name' do
  it 'resolves as the value of name attribute' do
    expect(result.dig('data', 'testing', 'name')).to eq('testing123')
  end
end

@jasl
Copy link

jasl commented Feb 28, 2019

a proposal that using Ruby accessor style to set graphql_name, description, etc.

e.g change graphql_name 'Query' to self.graphql_name = 'Query' and make graphql_name method a pure reader

@abhaynikam
Copy link
Contributor

@rmosolgo : Thanks for the awesome work 🎉. I would love to contribute to release 1.10.0.

Instrumentation: remove it

Can I start with this?

@rmosolgo
Copy link
Owner Author

rmosolgo commented Mar 8, 2019

Yes, sure! To be clear, it just means removing the docs for field instrumentation. I think query instrumentation has to stay, since there's no alternative (and afaik there doesn't have to be one, it's fine to keep as-is).

@abhaynikam
Copy link
Contributor

@rmosolgo : Should we go ahead and remove this? I think we can remove it unless people need this doc.

Dynamic definition: just remove it?

@rmosolgo
Copy link
Owner Author

rmosolgo commented Mar 9, 2019

👍 sounds good to me. Besides, since the class-based API is just classes, you can use plain old Ruby to metaprogram it (Class.new(GraphQL::Schema::Object) { ... }).

@rmosolgo
Copy link
Owner Author

(🚢 to Rubygems 🎉, but better data loading was pushed back 😖 )

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

No branches or pull requests

6 participants