-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/graphql scaffold #81
base: develop
Are you sure you want to change the base?
Conversation
from source.mutations.UpdateOrCreateSourceMutation import UpdateOrCreateSourceMutation | ||
|
||
|
||
class Query(SourceQueries, ObjectType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not everyone is a fan of inheritance like this. We can also write all the queries (and their resolvers here), but that usually ends up being a very long list.
|
||
|
||
class SourceQueries(ObjectType): | ||
sources = DjangoListField(SourceType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DjangoListField is a handy util from graphene_django
that has a built-in resolver. By default it returns all objects in the database. When we start to add filtering/sorting, we will have to write resolve_sources()
ourselves.
model = Source | ||
|
||
@classmethod | ||
def get_queryset( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually define get_queryset()
on every type. It is used to filter the queryset, most often based on the user making the request (obtainable via info.context.user
). It's not necessary (and in fact return queryset.all()
is the default implementation), but it is nice to be explicit about returning all objects of a particular type.
frontend/tsconfig.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not touch this file. Changes were made after running ng add apollo-angular
.
import { NgModule } from '@angular/core'; | ||
import { ApolloClientOptions, InMemoryCache } from '@apollo/client/core'; | ||
|
||
const uri = 'http://localhost:8000/api/graphql'; // <-- add the URL of the GraphQL server here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL is now hardcoded. I can move it to environment.ts
and environment.prod.ts
, but how does this work when we start deploying? What URL should I pick for the production backend?
I'm sure you have run into this issue before in other projects, so any advice would be helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, you use absolute URLs without a hostname (+ port), e.g. '/api/graphql'
.
If you're accessing the frontend development server on https://localhost:4200
, this will resolve as https://localhost:4200/api/graphql
. The development server includes a proxy configuration, which will redirect that to https://localhost:8000/api/graphql
, i.e. the backend port.
On production, you don't have this kind of proxying, as the endpoints for the frontend client and the backend API will have the same origin (e.g. https://myproject.org
and https://myproject.org/api/graphql
).
The example in the apollo-angular documentation uses a URL like this, did you try that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine overall, I added some minor comments.
My only real issue is that is looks like the graphql endpoint (including mutations) is currently fully accessible without authorisation. Is that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this app seem to be specifically handling the graphql portion of the API, perhaps graphql
would be a more appropriate name than api
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This app includes some files generated by startapp
which aren't used (admin
, models
, views
, tests
). Can you remove them?
import { NgModule } from '@angular/core'; | ||
import { ApolloClientOptions, InMemoryCache } from '@apollo/client/core'; | ||
|
||
const uri = 'http://localhost:8000/api/graphql'; // <-- add the URL of the GraphQL server here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, you use absolute URLs without a hostname (+ port), e.g. '/api/graphql'
.
If you're accessing the frontend development server on https://localhost:4200
, this will resolve as https://localhost:4200/api/graphql
. The development server includes a proxy configuration, which will redirect that to https://localhost:8000/api/graphql
, i.e. the backend port.
On production, you don't have this kind of proxying, as the endpoints for the frontend client and the backend API will have the same origin (e.g. https://myproject.org
and https://myproject.org/api/graphql
).
The example in the apollo-angular documentation uses a URL like this, did you try that?
from source.models import Source | ||
|
||
|
||
class DeleteSourceMutation(Mutation): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of this looks fine, but deleting sources is quite destructive, so it may be better to restrict this to the admin interface.
Part of #75
Basic implementation of a GraphQL API using Graphene (Django) en Apollo (Angular).
Don't forget to install dependencies and run
yarn codegen
when running the application frontend.This PR also adds a SourceType, a query
sources
and two mutations:DeleteSource
andUpdateOrCreateSource
, so there is something to test. Visitlocalhost:8000/api/graphql
to test them out.Still to do (in other PRs):
codegen.ts
andgraphql.module.ts
are hardcoded.I'll focus on writing more types and queries after this PR so we can start using them in the frontend when the views/components are there.