Add impl derive for graphql objects

[ForsakenHarmony]

https://gist.github.com/ForsakenHarmony/8d42d4963dec52d80853e48ff7401ef1#file-proc_macro-rs

[theduke]

Love it. Exactly what I wanted to add.

Sadly, this will have to wait until proc macros hit stable.
We could feature gate it behind a nightly feature though.

[theduke]

I will do a more thorough review soon, but some short hints:

• we can probably use https://crates.io/crates/proc-macro-hack to make it work on stable

It would be nice to specify the context also inside the regular code.
Something in the vain of:

#[graphql_object]
impl GraphQLObject for MyType<Context=SomeType> {
}

[ForsakenHarmony]

proc-macro-hack is for function like macros?
this is an attribute proc macro

and sure that would be something to think about

[ForsakenHarmony]

welp, I just upgraded this to the latest syn and quote as well

[ForsakenHarmony]

well the test was broken before because you pass --all-features which includes nightly

[ForsakenHarmony]

Should I also add

#[graphql_object]
impl GraphQLObject for MyType<Context=SomeType, Interfaces=(SomeInterface, AnotherInterface)> {
}

(tuples being the only way to have multiple items with valid syntax)

[ForsakenHarmony]

@theduke ?

[LegNeato]

Does proc_macro being stabilized change this? Of course, it will still have to be gated because we don't want to bump our rust requirement that high yet so maybe not...

[ubnt-intrepid]

I think it is not good to specify the context type as a bounds of associated types.
The syntax is different to the bindings of associated type which is usually used in the impl and hence not intuitive.

I thinks it is prefer to be written as follows:

#[graphql]
impl MyType {
    type Context = ();
    ...
}

In order to avoid confusion, we should clarify the specification of the attribute-style macro to be created...

[ForsakenHarmony]

Good idea

[theduke]

@ForsakenHarmony are you motivated to finishing this up?

[ForsakenHarmony]

If you tell me what exactly you want

[theduke]

A few things left to do:

• rebase
• handle the new custom scalar generic on GraphQLObject
• remove the nightly cfg stuff
• add a few tests similar to the other custom derive tests in integration_tests/juniper_tests/codegen
• specify context with type Context = *
• require impl GraphQLObject for X (see below)

I can absolutely help out with any of those, you just say if and what you are able to tackle. ( it's great that you started this in the first place!)

===
I think it would be nice to make it clear in the code that you are implementing GraphQLObject, so it could look something like this:

#[graphql_object]
impl GraphQLObject for MyType {
  type Context = MyCtx;

  ...
}

[ForsakenHarmony]

I'll take a look

[ForsakenHarmony]

impl GraphQLObject for MyType { makes sense for the Context type, but not for "dynamic" functions that are not in the trait itself

[codecov-io]

Codecov Report

Merging #181 into master will decrease coverage by 0.74%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
- Coverage   87.61%   86.86%   -0.75%     
==========================================
  Files         101      102       +1     
  Lines       14415    14539     +124     
==========================================
  Hits        12629    12629              
- Misses       1786     1910     +124

Impacted Files	Coverage Δ
juniper_codegen/src/derive_object_impl.rs	0% <0%> (ø)
juniper_codegen/src/lib.rs	2.27% <0%> (-0.3%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 036152e...4c54425. Read the comment docs.

[Swoorup]

Does this allow graohql queries/mutation to be split across multiple files?

[ForsakenHarmony]

Nope

[theduke]

@ForsakenHarmony what was contained in your recent push, and what's still TODO?

[ForsakenHarmony]

it was just a rebase, pretty much everything, because you didn't respond before

[theduke]

@ForsakenHarmony

impl GraphQLObject for MyType { makes sense for the Context type, but not for "dynamic" functions that are not in the trait itself

Yeah I can somewhat agree with this, impl GraphQLObject for T might be confusing too since you are not actually implementing the trait.

I'm fine with the current solution.

Also, I'd love to get this finished since I really want to use it myself. ^^

Just tell me what you want to tackle and I can do the rest.

[ForsakenHarmony]

Oh right, this is a thing

But I guess my ide would also show an error for the rest of the functions if it was a trait implementation

[theduke]

Mhm, yeah, I'm afraid we won't be able to solve that in a reasonable fashion.

It would not be a problem for RLS based IDEs because they only handle the expanded code,
but it would break Clion/Idea.

I think for now we have to live with it.

In the future we could provide a regular proc macro that replaces the current graphql_object! which can be used by anyone who is annoyed by the errors.
It would be the exact same code so it's no overhead to support it.

[theduke]

Ah, I just noticed something else due to the above issue:

It would be quite confusing to have both a graphql_object! macro and a gql_object proc macro.

We should find a name that distinguishes the two clearly.

Since with edition 2018 macros are not global and can be imported, my preference would be: impl_object.

#[juniper::impl_object]
impl Query{
  ...
}

Anyone have other thoughts here?

We could rename it to just object in a future major version / 1.0.

[ForsakenHarmony]

what's the new custom scalar generic on GraphQLObject?

I'll probably do that and the type Context but can't say if I'll have time before the weekend

[theduke]

The GraphQLType has a generic argument for the scalar type now.

We should support setting this, I'd reckon with type ScalarValue = X.

[ubnt-intrepid]

Since the scalar type is defined as an argument (not an associated type), it should be able to write as follows:

#[juniper::impl_object]
impl<S> GraphQLType<S> for Query
where
    S: juniper::ScalarValue + MyCustomBound,
{
  ...
}

[ForsakenHarmony]

well, it might be better if you just implement it, don't have too much time and am not entirely sure what the best way is

[theduke]

Continued in #333 due to Github being buggy.

[ForsakenHarmony]

seems like you accidentally force pushed master or smth, making this empty

did that once as well

[theduke]

Wasn't an accident, I squashed your commits together.
Apparently Github can't handle it if the PR is in the master branch and the original base commit is lost.

Jedenfalls nochmal danke. ;)