Skip to content
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

chore: Add all screen queries to routes.tsx for prefetching #11709

Conversation

olerichter00
Copy link
Contributor

@olerichter00 olerichter00 commented Mar 18, 2025

Description

This PR adds missing screen queries to routes.tsx. The queries will be used for prefetching.

🤖 Generated with Claude Code

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • Add all screen queries to routes.tsx for prefetching - ole

Need help with something? Have a look at our docs, or get in touch with us.

Sorry, something went wrong.

@olerichter00 olerichter00 marked this pull request as draft March 18, 2025 12:21
@olerichter00 olerichter00 changed the title chore: Add missing screen queries to routes for prefetching chore: Add all screen queries to routes.tsx for prefetching Mar 18, 2025
- Added ArtistShowsScreenQuery to ArtistShows.tsx and updated routes.tsx
- Added ViewingRoomArtworkScreenQuery to ViewingRoomArtwork.tsx and updated routes.tsx
- Exported screen queries for use in route definitions to enable data prefetching
@olerichter00 olerichter00 changed the title chore: Add all screen queries to routes.tsx for prefetching chore: Add screen queries to routes.tsx for prefetching Mar 18, 2025
@olerichter00 olerichter00 self-assigned this Mar 18, 2025
@olerichter00 olerichter00 requested a review from damassi March 18, 2025 14:05
@olerichter00 olerichter00 marked this pull request as ready for review March 18, 2025 14:05
@ArtsyOpenSource
Copy link
Contributor

This PR contains the following changes:

  • Dev changes (Add all screen queries to routes.tsx for prefetching - ole - olerichter00)

Generated by 🚫 dangerJS against f99199d

@olerichter00 olerichter00 changed the title chore: Add screen queries to routes.tsx for prefetching chore: Add all screen queries to routes.tsx for prefetching Mar 18, 2025
@@ -33,7 +33,7 @@ export const ArtQuizResults = ({ isCalculatingResult }: { isCalculatingResult?:
)
}

const artQuizResultsQuery = graphql`
export const ArtQuizResultsScreenQuery = graphql`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose this naming pattern because it's the one that is currently used for most queries. However, I'm open to changing it.

@@ -442,6 +492,7 @@ export const artsyDotNetRoutes = defineRoutes([
path: "/artist/:artistID/shows",
name: "ArtistShows",
Component: ArtistShowsQueryRenderer,
queries: [ArtistShowsScreenQuery],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases like this query, we need queryVariables as well so this would work properly. So we might need to export it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. In this case, the variable :artistID will be taken from the path, but for other routes, we will need to either add the variables here or set the defaults within the GraphQL query.

I'm not sure if I can automate this work. So far, I have manually tested whether the prefetching worked and added the variables in case it hasn't... 🤔

Copy link
Member

@MounirDhahri MounirDhahri Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining.
this past week with infinite-discovery I tried prefetchQuery and it failed because it's missing the query variables and only worked when I added them.

Let's move with this then and keep an eye on Sentry

Copy link
Member

@damassi damassi Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should go the the route approach as that's our centralized hub which contains all screen dependencies and avoid adding details to our queries that can be handled on the JS side. Over in force this is how we do it:

So as things grow, and if we ever need to add anything more, the routes file / route item is the only place you need to go, and then just call findRoutesByPath and pass it the route path in question. Has worked well over there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I received some more warnings related to this that I didn't expect.
Screenshot 2025-03-28 at 18 10 56

@olerichter00 olerichter00 merged commit ba88a10 into main Mar 18, 2025
7 checks passed
@olerichter00 olerichter00 deleted the olerichter00/add-missing-screen-queries-to-routes-for-prefetching branch March 18, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants