feat: remove usage of GraphQLContext interface #1580
Conversation
| @Deprecated(message = "The generic context object is deprecated in favor of the context map", replaceWith = ReplaceWith("generateContextMap(request)")) | ||
| suspend fun generateContext(request: Request): Context? = null | ||
|
|
||
| interface GraphQLContextFactory<Request> { |
There was a problem hiding this comment.
Maybe we can default to GraphQLContext.of() to be extra clear what the object here is
There was a problem hiding this comment.
Actually, we could make a MUCH more significant change and instead start using GraphQLContext as the type everywhere instead of Map<*, Any>?.
We end up mapping back to a builder anyway in GraphQLRequest.toExecutionInput() so if we let everyone just call the builder themselves to modify that might be eaiser
There was a problem hiding this comment.
That would require everyone to migrate though, and almost certainly break existing code.Tthis is at least a breaking change but will possibly only require removal as someone could have migrated to the new Map methods
There was a problem hiding this comment.
i think for simplicity it was decided to just return a Map and let the GraphQLContextFactory build the GraphQLContext using that map.
i would keep it like that for simplicity and to not add another breaking change bc of the potential change of the return type
There was a problem hiding this comment.
since it is a breaking change anyway -> lets approach it from a new user perspective, what would be a better user experience?
There was a problem hiding this comment.
So DataFetchingEnvironment.getGraphQlContext returns GraphQLContext not Map<> so I think it would sense that the factory sets an object and that is the one you get back out.
There was a problem hiding this comment.
applied the updates to return a GraphQLContext object instead of a Map
.../main/kotlin/com/expediagroup/graphql/server/extensions/dataFetchingEnvironmentExtensions.kt
Show resolved
Hide resolved
|
Make sure you add the proper PR labels |
dariuszkuc
added
changes: major
| } | ||
|
|
||
| /** | ||
| * Helper method to get values from a registered DataLoader. | ||
| */ | ||
| @Deprecated( |
There was a problem hiding this comment.
These methods are useful. I don't think we need to deprecate these. I was referring to the methods to get values from context directly on DataFetchingEnvironment, ie lines 57+ in this file
There was a problem hiding this comment.
ahh ok, well we added those extensions bc of a feature request, where ppl stores in context using KClass, so is definetely more idiomatic do to
dataFetchingEnvironment.getFromContext<CoroutineScope>()than
dataFetchingEnvironment.graphQLContext.get(CoroutineScope::class)There was a problem hiding this comment.
But with the extension methods here you can do this
dataFetchingEnvironment.graphQLContext.get<CoroutineScope>()There was a problem hiding this comment.
i still find it useful, but yes, it looks like an unneeded indirection, i can remove these in a separate PR.
There was a problem hiding this comment.
I don't think they should be deleted just yet, maybe just deprecated, but yes that can be a separate PR
📝 Description
Removing all references of
GraphQLContextin favor of context map.GraphQL context map is the official approved context mechanism.
🔗 Related Issues
#1548