Skip to content

Attempt first naive version of async union #552

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

xla
Copy link
Contributor

@xla xla commented Mar 6, 2020

This addreses #549 as we ran into the lack of support for unions when trying to migrate to async/await. This is a severe case of i-have-no-clue what I'm doing. As mostly inspired by the enum implementation copy-pastad code at the moment, I look very much for feedback on how to make the async support proper and according to the standards of the crate. So don't go easy here, thanks.

@LegNeato
Copy link
Member

LegNeato commented Mar 7, 2020

Thanks for the PR! 🍻 Sadly it looks likes the tests are failing.

@arlyon
Copy link
Contributor

arlyon commented Mar 24, 2020

All the tests pass on my machine with a small change to a lifetime:

diff --git a/juniper/src/macros/tests/union.rs b/juniper/src/macros/tests/union.rs
index 6dfbfe75..089ea3b2 100644
--- a/juniper/src/macros/tests/union.rs
+++ b/juniper/src/macros/tests/union.rs
@@ -57,7 +57,7 @@ impl CustomName {
 }
 
 #[crate::graphql_union_internal]
-impl<'a> WithLifetime<'a> {
+impl<'b> WithLifetime<'b> {
     fn resolve(&self) {
         match self {
             Concrete => match *self {

@LegNeato
Copy link
Member

I'll have to look more later, but one should be able to name lifetimes whatever. So likely the macro generated lifetime needs to have a name like '__a to avoid conflicts.

@LegNeato
Copy link
Member

Also note some union tests are turned off:

// FIXME: re-enable

They need to be turned back on and pass.

@xla xla closed this May 10, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants