feat: Add featuredFairsConnection #6421
Conversation
|
|
|
||
| const now = moment.utc() | ||
|
|
||
| const { body: unfilteredRunningFairs } = await fairsLoader(gravityOptions) |
There was a problem hiding this comment.
This will return a default of 10 records from the gravity endpoint (as no pagination arguments are specified).
| allFairs = allFairs.concat(closedFairs) | ||
| } | ||
|
|
||
| const totalCount = allFairs.length |
There was a problem hiding this comment.
This isn't really doing pagination as 'all fairs' data is being assembled here, and only then 'sliced' over by the incoming connection arguments.
In general connections are always better than lists (as they're future proof and can support some 'fake' pagination or real pagination) - but in this case, maybe the schema should just be the full list? (featuredFairs w/ no connection).
Just calling out b/c anytime we do something 'quirky' with pagination, or kind of fake it by filtering and doing pagination not on the backend server - my ears perk up!
| const runningFairs = filter(unfilteredRunningFairs, (fair) => { | ||
| const startAt = moment.utc(fair.start_at) | ||
| return now.isAfter(startAt) | ||
| }) |
There was a problem hiding this comment.
suggestion / non-blocking: The /api/v1/fairs API endpoint supports a status filter of running. Should we use that instead of active: true and this inline filter?
Not sure if this is used right now, but this fairs query has similar arguments.
| const now = moment.utc() | ||
| const { size, offset, page } = convertConnectionArgsToGravityArgs(args) | ||
|
|
||
| const numberOfFairsToFetch = size + offset |
There was a problem hiding this comment.
suggestion: I think offset (along with size) can be directly passed to our pagination-supported API endpoints (source).
If the initial fairsLoader response returned fewer running fairs than the client's requested size, that's when we can trigger the backfill.
Something like:
const backfillSize = size - runningFairs.length
if (backfillSize > 0 && backfillSize > 0) {
// request and add backfill fairs
}| const closedFairs = filter(unfilteredClosedFairs, (fair) => { | ||
| const endAt = moment.utc(fair.end_at) | ||
| return now.isAfter(endAt) | ||
| }) |
There was a problem hiding this comment.
question: Similar to the question about running above – do we not trust Gravity to only return "closed" fairs? I wonder why this inline filter is needed...
| const totalCount = | ||
| allFairs.length < numberOfFairsToFetch | ||
| ? allFairs.length | ||
| : MAX_NUMBER_OF_FAIRS |
There was a problem hiding this comment.
I wonder if it's possible to return the combined total counts from both Gravity requests to return an accurate number here.
Something like:
const { body: runningFairs, headers: runningFairsHeaders } = await fairsLoader({
size,
offset,
status: "running",
total_count: true,
})
const { body: closedFairs, headers: closedFairsHeaders } = await fairsLoader({
size: backfillSize,
status: "close",
total_count: true,
})
const totalCount = parseInt(runningFairs["x-total-count"] || "0") + parseInt(closedFairs["x-total-count"] || "0")
olerichter00
force-pushed
the
olerichter00/ONYX-1508/add-featuredfairsconnection
branch
from
44730a3 to
21f710d
Compare
Addresses ONYX-1508
Description
This PR creates the new root field
featuredFairsConnection. The connection replaces the Fairs home module and is used as a root field (for the new "Featured Fairs" screen) and the "Featured Fairs" home view section. Both the new root connection and the section share the resolver and return the same list of fairs.Query:
{ me { email internalID } homeView { section(id: "home-view-section-featured-fairs") { __typename internalID ... on HomeViewSectionFairs { fairsConnection(first: 10) { totalCount edges { node { name startAt } } } } } } featuredFairsConnection(first: 30) { totalCount edges { cursor node { name startAt endAt } } } }