CHECK-395: Consolidate project page fetch into a single GraphQL query#2910
Open
amy-at-kickstarter wants to merge 2 commits into
Open
CHECK-395: Consolidate project page fetch into a single GraphQL query#2910amy-at-kickstarter wants to merge 2 commits into
amy-at-kickstarter wants to merge 2 commits into
Conversation
| slug: .someOrNil(projectParam.slug) | ||
| ) | ||
|
|
||
| switch (projectParam.id, projectParam.slug) { |
Contributor
Author
There was a problem hiding this comment.
This extra mocking existed because the return type, ProjectPamphletData, was a tuple and not decodable. I made it a struct and decodable which let me delete all of this.
| return SignalProducer(value: envelope) | ||
| } | ||
|
|
||
| internal func fetchProject(project: Project) -> SignalProducer<Project, ErrorEnvelope> { |
Contributor
Author
There was a problem hiding this comment.
This method is unused. Since it used the query I renamed, I just deleted it instead of fixing it. (I also deleted it from ServiceType and Service).
| ) | ||
|
|
||
| return SignalProducer(value: data) | ||
| public struct ProjectPamphletData: Decodable { |
Contributor
Author
There was a problem hiding this comment.
I'm not entirely sure we need this envelope at all. But for now, making it a struct so it's easier to handle.
Collaborator
Generated by 🚫 Danger |
|
|
||
| static func project( | ||
| from data: GraphAPI.FetchProjectBySlugQuery.Data, | ||
| internal static func project( |
Contributor
Author
There was a problem hiding this comment.
Tagged this as internal because it's only called by the method above, projectProducer(from:configCurrency:)
amy-at-kickstarter
requested review from
a team and
scottkicks
and removed request for
a team
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📲 What
Combine
FetchProjectByIdQueryandFetchProjectBySlugQueryinto one query,FetchProjectByParamQuery.🤔 Why
A project page can be fetched in two ways: by slug, or by pid. This was originally defined as two separate GraphQL queries:
FetchProjectBySlugQueryandFetchProjectByIdQuery, respectively.Because Apollo makes separate types for each query, we had different parsing code to handle each of the two query types. This led to a bunch of duplication, and makes it difficult to update this query.
By combining these into a single query - in which both
pidandslugare optional - we can delete all this duplicate code and make it easier to handle. We use the enum typeParamto make sure you always create this query correctly.