-
Notifications
You must be signed in to change notification settings - Fork 17
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
Render news articles using GraphQL #3447
Conversation
d94dad1
to
515a0aa
Compare
515a0aa
to
4896c26
Compare
4896c26
to
d83bd0c
Compare
d83bd0c
to
999d3c2
Compare
999d3c2
to
2f9cecf
Compare
2f9cecf
to
d77199c
Compare
d77199c
to
9f0abcb
Compare
9f0abcb
to
1f97173
Compare
1f97173
to
f5d1bb1
Compare
f5d1bb1
to
5983a10
Compare
5983a10
to
7399d93
Compare
7399d93
to
9e25802
Compare
9e25802
to
442f3d2
Compare
442f3d2
to
10ae11c
Compare
10ae11c
to
3d92bba
Compare
8bbf5f4
to
6888f79
Compare
6888f79
to
b8437e8
Compare
b8437e8
to
7a02425
Compare
7a02425
to
04a1b56
Compare
04a1b56
to
f5b7ecd
Compare
f5b7ecd
to
a9da5b1
Compare
a9da5b1
to
eb06e37
Compare
eb06e37
to
ea1da19
Compare
ea1da19
to
c36541a
Compare
@content_item = if use_graphql? | ||
schema_name_result = Services | ||
.publishing_api | ||
.graphql_content_item(Graphql::SchemaNameQuery.new(content_item_path).query) | ||
|
||
if schema_name_result["schema_name"] == "news_article" | ||
load_content_item_from_graphql | ||
else | ||
load_content_item_from_content_store | ||
end | ||
else | ||
load_content_item_from_content_store | ||
end |
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.
thought: two requests issue
I suppose if we were to switch over to GraphQL for everything, we might be able to remove the schema name query and do what I suggested in the meeting with Richard and Martyn: have a single big query that specifies what we want for each type using the ...Type
syntax. We'd have to weigh up whether one big query is better than one very small one and then the normal schema-specific one
I think this could work with both the current more strongly typed implementation and the Edition god type alternative. With the latter, we could have types that don't themselves define any fields but do at least map document types to GraphQL types, like you've already done for news articles: https://github.com/alphagov/publishing-api/blob/main/app/graphql/types/news_article_type.rb. Then whichever ...Type
section of the query matches would be executed
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.
Would it be gross to change this to just be
@content_item = use_graphql? ? load_content_item_from_graphql : load_content_item_from_content_store
and then at the top of the load_content_item_from_graphql
method we call a new method that checks for the content type and either carries on using graphql or just returns load_content_item_from_content_store
I know that it hides a bit of functionality but it would make this bit of code look nicer
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've reduced this down to one GraphQL query now. We'll need to think about the query a bit more once any changes to the typing in Publishing API have been made and when we add GraphQL support for further document types rendered by Government Frontend.
This allows us to toggle the use of GraphQL by environment, in the same way as Collections: alphagov/collections@bc1fedf.
c36541a
to
90f8d4d
Compare
This retrieves the content from Publishing API, instead of Content Store, using a GraphQL query. Note this has been done in the way that reduces the number of changes needed in this application.
90f8d4d
to
3f85edc
Compare
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.
Thanks Bruce
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.
Looks good to me
This switches the rendering of news articles to use Publishing API's GraphQL endpoint, only when the feature flag is enabled.
The code here (and in the associated Publishing API PR: alphagov/publishing-api#3008) has been written in such a way that the least number of changes have been needed in this app, as this is only a proof of concept for now. If we decide to use GraphQL for everything going forward, we should revisit the structure of content returned by GraphQL.
Also note that GraphQL powered queries rely on an initial request to Content Store, since we need to know the content type before deciding whether to use GraphQL.
Furthermore it only renders one level of parent taxon. We will have follow-up work to change how the taxonomy is presented by GraphQL, as it's recursive nature doesn't fit the current implementation.
Trello card