-
Notifications
You must be signed in to change notification settings - Fork 437
Open
Labels
easyenhancementImprovement of existing features or bugfixImprovement of existing features or bugfixk::apiRelated to API (application interface)Related to API (application interface)
Milestone
Description
Is your feature request related to a problem? Please describe.
Contexts have some boilerplate, you have to implement juniper::Context
which is usually an empty marker trait. The error message when you don't isn't obvious, see #327.
Describe the solution you'd like
It might be good to have a #[derive(GraphQLContext)
that does the boilerplate for you. It won't help the error message, but perhaps when people are copying and pasting or reading docs they will be less likely to miss it.
Metadata
Metadata
Assignees
Labels
easyenhancementImprovement of existing features or bugfixImprovement of existing features or bugfixk::apiRelated to API (application interface)Related to API (application interface)
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
theduke commentedon Mar 6, 2019
Actually, I think we could just deprecate the
Context
trait completely and remove the trait bound.There is no real reason for it to be there.
theduke commentedon Mar 6, 2019
I guess the only reason for it's existence would be that you can't use a type as context in the macros that isn't intended to be used as a context.
But that would produce compile time errors anyway once you actually construct the schema.
LegNeato commentedon Mar 24, 2019
Do we want to reserve the trait for future extensibility? If we remove the requirement and later need it for implementation specifics, it would be a breaking change.
davidpdrsn commentedon Mar 31, 2019
@LegNeato what sort of future extensibility are you thinking of? I imagine adding methods to the trait would also be breaking change.
I'm also in favour of deprecating it.
theduke commentedon May 16, 2019
I don't see a reason to ever add methods on context.
More state keeping or functionality would be added to the
Executor
.There is a consideration with futures support though: we will probably need a
Send + Sync + Clone
bound on context!With that in mind, we may want to add those constraints onto the
Context
trait instead of litterling them around the code everywhere.davidpdrsn commentedon May 18, 2019
I'm not that familiar with futures, but why
Clone
?The app I'm working on has a context like this:
Where
DbCon
isPooledConnection
comes fromr2d2
and is notClone
. So I can't easily make this contextClone
.I could instead store the connection pool itself in the context. The pool is
Clone
because it just wraps andArc<SharedPool>
. That feels like it would create a lot of churn on the pool though. If each db query would have to first get a new connection.I also see that
PgConnection
from Diesel is also notSync
🤔theduke commentedon May 18, 2019
That's a necessary result from concurrency/parallelism.
We'll need to clone the Context often.
You could still use a single DB connection per query by wrapping it in
Arc<Mutex<Connection>>
.(plus a convenience
get_connection()
method on the Context)Note that the diesel connections are not
Sync
, but areSend
.But that would of course negate any benefit if most of your resolvers need the db.
davidpdrsn commentedon May 18, 2019
Makes sense. Seems to still be early days for async in Diesel.
petoknm commentedon Jun 27, 2019
I think we could use a trait alias for this in the future for async. Having an empty trait doesn't make much sense to me. I will try to make a PR to remove it.
theduke commentedon Jun 27, 2019
@petoknm great that you want to help out, I would hold off on this for now though until I publish my async/await branch and we have a clearer picture of what will be needed.
petoknm commentedon Jun 27, 2019
Cool, I didn't know that work on async support has already started.