@kaqqao Please add the default implementation again: We are on Java 8 now and if that helps not breaking stuff, we will take it.

Because changing such a central interface we should avoid almost always, I prefer adding a better alternative side by side.

Thanks!

@andimarek The only issue with adding default to the new TypeResolver method and deprecating the current is that it forces implementors to still implement the deprecated one:

public interface TypeResolver {

    default GraphQLObjectType getType(TypeResolutionEnvironment env) { //backward compatible :)
        return getType(env.getObject());
    }

    @Deprecated
    GraphQLObjectType getType(Object object); //still forces implementation :(
}

Not sure I got you on the last bit, do you mean you'd like me to add the TypeDecider interface I've described (can change name if you wish) in addition to default or you think default is enough`?

Sorry to give you a bum steer here. We also have another PR in the works that uses the "parameters pattern". re: #353

I want to get this in place before this PR as the naming of the parameters is preferred.

OK, I'll rework this one then after #353 is merged. Do drop a line if you an opinion on default/TypeDecider thing.

I think @andimarek wants you to use the default keywords because we can now that we are on Java 8. So yes people will have to implement the old as always but they can opt into the "default" implementation

@kaqqao @bbakerman I would try to avoid such a big breaking change: TypeResolver is probably implemented quite often.
Maybe we could subclass TypeResolver and therefore detect if somebody is using the new resolve method or the old? Not sure this is a great idea though.

@andimarek @bbakerman Here's a suggestion mixing the approaches mentioned so far:

public interface TypeResolver {

    GraphQLObjectType getType(Object object);
    
    default GraphQLObjectType getType(TypeResolutionEnvironment env) {
        return getType(env.getObject());
    }
}

public interface AdvancedTypeResolver extends TypeResolver { //could maybe use a better name

    @Override
    GraphQLObjectType getType(TypeResolutionEnvironment env);

    @Override
    default GraphQLObjectType getType(Object object) {
        //Could instead delegate to the method above, but I think an exception is better as library code never actually calls this method
        throw new UnsupportedOperationException("AdvancedTypeResolver does not support simple type resolution");
    }
}

While a bit convoluted, this solves the conundrum rather effectively.
All existing TypeResolvers will keep working as usual and the new code can choose to use AdvancedTypeResolver instead, implementing only the new method. Library code will only ever call the new method in all cases (already true in this PR). None of the builders that accept TypeResolver or any other code need to change, unlike in my original TypeDecider approach.

Didn't want to push it like this before I hear your comments.

@kaqqao Thanks ... this goes in the direction I would prefer. But do we need the new method in TypeResolver with the default Implementation? This seems confusing to be honest.

@andimarek It is confusing, I agree. I added it to remove the need for any other changes. If I remove it, I have 2 options:

• Do instanceof check inside resolveType:

typeResolver instanceof AdvancedTypeResolver ? ((AdvancedTypeResolver) typeResolver).getType(env) : typeResolver.getType(env.getObject);

• Make GraphQLInterfaceType and GraphQLUnionType always wrap the given TypeResolver into AdvancedTypeResolver (the same approach I took for my TypeDecider idea)

Do you think one of these is the way to go?

@kaqqao - since we changed the parameter calling structure quite a lot this has large conflicts.

But I forked your fork and made a new commit which is mostly your code but with fix ups

Its here

#395

I think I will close this one and use the other one (which is essentially yours) as the way forward

Awesome! Do you need me to do anything at this point?

Now merged