Extend the TypeFunction API to be more flexible (Issue #81)#82
Conversation
| * This interface will not extend {@link BiFunction} in a future release. | ||
| */ | ||
| @Deprecated | ||
| default GraphQLType apply(Class<?> aClass, AnnotatedType annotatedType) { |
There was a problem hiding this comment.
renaming apply to buildType makes sense as a counterpart to canBuildType but does it need to be deprecated
Notice how much "disruption" you have caused in all the places in order to get a better name?
I would leave apply as a synonym and cause less disruption and live with the lesser name and simply document it as a synonym for the better named "buildType"
Either way this is a breaking change since Collection<Class> getAcceptedTypes(); is SPI and boolean canBuildType(Class aClass, AnnotatedType annotatedType); has been introduced.
So all existing type functions are broken by this.
If we are happy to break API then perhaps this disruption is ok.
Can we make it
default boolean canBuildType(Class<?> aClass, AnnotatedType annotatedType) {
return getAcceptedTypes().contains(aClass);
}
say?
That way I think it might stay API compatible yet able to be overridden
There was a problem hiding this comment.
or we make it a full API breakage and make a new major version
There was a problem hiding this comment.
It's a disruptive change, but in the end we won't have a BiFunction as the type builder. I think making this a breaking change with a major version bump is a suitable choice.
| } | ||
|
|
||
| public DefaultTypeFunction() { | ||
| registry = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
why take this out. Or at least make a concurrent list replacement.
People use this staticalyl today in some cases and hence thread visibility is important
There was a problem hiding this comment.
I'll replace it with a CopyOnWriteArrayList
|
This is also moving away from the Is this seen a desireable? I personally approve since it allows for more than 1 TypeFunction registry and so on but I am asking for consensus from other maintainers?? |
see #81