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

RFC: Support for multiple queries in Faust #1628

Closed
blakewilson opened this issue Nov 1, 2023 · 27 comments
Closed

RFC: Support for multiple queries in Faust #1628

blakewilson opened this issue Nov 1, 2023 · 27 comments
Assignees
Labels

Comments

@blakewilson
Copy link
Contributor

blakewilson commented Nov 1, 2023

Motivation

Users may want separate queries in their Faust templates instead of one large query to render a template. For example, a Faust template may have a query for the specific post it is fetching (e.g. query GetPost), but also have a query that fetches layout related items like menus and general settings (e.g. query GetLayout). Currently in Faust, you must merge these two queries into one and include it in your Component.query. However, this could be a disadvantage from a caching perspective as you may be re-requesting data that isn't necessarily stale. Additionally, combining everything into a single query is not the best developer experience.

API Changes

We will add an additional property to a Faust template called Component.queries, that can accept an array of queries, for example:

export default function Component(props) {
}

Component.queries = [
  {
    query: GET_POST_QUERY,
    variables: ({ databaseId }, ctx) => {
      return {
        databaseId,
        asPreview: ctx?.asPreview
      }
    }
  },
  {
    query: GET_TEMPLATE_QUERY,
    // Variables are not required
    variables: () => {}
  }
]

The existing Component.query functionality will be preserved. A Faust template could either use Component.queries to fetch multiple queries or Component.query to fetch a single query, but not both.

This data could then be retrieved in the Component using the useFaustQuery hook:

export const GET_POST_QUERY = gql`
  query GetPost {
    ...
  }
`

export const GET_TEMPLATE_QUERY = gql`
  query GetTemplate {
    ...
  }
`

export default function Component(props) {
  // Pass the query (DocumentNode) as an argument to useFaustQuery to retrieve the data
  const { post } = useFaustQuery(GET_POST_QUERY);

  return(
    <>{post.title}</>
  )
}

Component.queries = [
  {
    query: GET_POST_QUERY,
    variables: ({ databaseId }, ctx) => {
      return {
        databaseId,
        asPreview: ctx?.asPreview
      }
    }
  },
  {
    query: GET_TEMPLATE_QUERY,
    variables: () => {}
  }
]

Drawbacks

  1. One potential drawback of this design is that all Component.queries will share the same variables, meaning if two queries use different variables, all necessary variables need to be returned from the Component.variables property.

Queries with their own variables were added to the RFC and are now supported.

  1. Queries can not be "chained". As in, data from one query can not be used to make a subsequent query; they are all executed at the same time.

How to Contribute

We are Interested in hearing your thoughts on this and any possible enhancement's that you want to see materialized. Do these API changes and conventions make sense?

@blakewilson blakewilson added the RFC label Nov 1, 2023
@JEverhart383
Copy link

Thanks for writing this up @blakewilson. I've got a few questions before I offer my thoughts. In the case where a user specifies multiple queries what gets resolved as props and is it required to essentially make another query using the useFaustQuery hook? I'd expect props to be an array of query data in this case.

@blakewilson
Copy link
Contributor Author

blakewilson commented Nov 1, 2023

Hey @JEverhart383, thanks for bringing this up! The data resolved from Component.queries will be returned in props in the form of props.__FAUST_QUERIES__. The property is an object such that the key is the SHA256 hash of the query, and the value is the query data result. For example, this may be what props.__FAUST_QUERIES__ returns if you have two queries:

{
  "4dd03f403403d6b001b160716df5990e53bafa4da78273d75d9cb2e65c622b0e": {
    "generalSettings": {
      ...
    },
    "headerMenuItems": {
      ...
    },
    "footerMenuItems": {
      ...
    }
  },
  "641f71bbfe48f0b065b5512d95398cf0d295538dc15b8fc519bf8232c573098f": {
    "post": {
      ...
    }
  }
}

The useFaustQuery hook is only used to retrieve the correct query from props.__FAUST_QUERIES__ (It does no data fetching). Since these queries are only locatable by their hashed equivalent, we decided to abstract that functionality so the user doesn't have to.

@jasonbahl
Copy link
Contributor

@blakewilson I think each query in the array of queries should specify it's own variables instead of use shared variables.

Consider 2 root queries that are imported to the template:

query GetRecentPosts( $first: Int ) {
  posts( first: $first ) {
    nodes { 
      id 
      title
    }
  }
}
query GetMenuItems( $first: Int ) {
  menuItems( first: $first ) {
    nodes { 
      id 
      label
      parentDatabaseId
    }
  }
}

I would like to pass a different $first variable to each, and if they were receiving shared variables, then they would both get the same number of items.

This is just one example, but I could probably come up with a lot more examples where shared variables could/would lead to unexpected bugs.

I think the array of queries should look more like:

Component.queries = [
   { 
     query: GET_POST_QUERY,
     variables: ({ databaseId }, ctx) => { ... },
     possibleFutureContextToSendToWPGraphQLForThingsLikePersonalization: () => {}
   }, 
   { 
     query: GET_TEMPLATE_QUERY,
     variables: ({ databaseId }, ctx) => { ... },
     possibleFutureContextToSendToWPGraphQLForThingsLikePersonalization: () => {}
   }
]

This would allow for components to have a bit more control over their behavior at their own component level.

@jasonbahl jasonbahl reopened this Nov 1, 2023
@jasonbahl
Copy link
Contributor

I accidentally clicked "close with comment" instead of "comment" 🤦🏻‍♂️. re-opened.

@JEverhart383
Copy link

Thanks for the clarification @blakewilson. Historically, the use of an additional hook call to resolve data has confused more people than accessing it directly via props but the hook actually seems like the clearer option in this case. I also assume that this API would be additive, meaning that Component.query would take precedence if both are defined. If that's true, this seems like a helpful addition that should help folks achieve much better cache granularity using the pages directory.

This may not be the place for it, but given all the inflight work on app router and the template hierarchy, I think it's worth calling this out here. It's clear that one large query has downsides to multiple smaller queries, but I think this has a lot to do with how the template system works currently. The frustration I hear most often from users is some variation of this question from Discord: What is the easiest way of implementing header and footer that will be shared among all pages?

Right now, most people end up reimplementing a header and footer component inside of each template, which means that its data requirements are either expressed in that template's query or passed in via props. The more I play around with RSCs, I wonder if there is a better way to organize these site parts so that my template's data requirements don't include "global" data as much as possible. When I think about traditional WP PHP templating, get_header and get_footer serve this purpose so that in most cases, my template doesn't need to concern itself with those parts and really focuses on the data and presentation that comprises the main section of content. Maybe this is a layout convention, or a pattern of colocation and an API for dynamically including site pieces based on the seed query result, and maybe I'm overthinking all of this and it just goes away with RSCs ???

But I think it's worth talking about here because of how this nuance of the current template model manifests itself in user feedback. It's rarely, "I want the be able to have multiple queries", it's that "I want some way to avoid repeating 'global' site components" which is ultimately why we need multiple queries.

FWIW, allowing multiple queries is the best way for us to improve the current thing, so I'm 100% on board with that addition to the API.

@blakewilson
Copy link
Contributor Author

@blakewilson I think each query in the array of queries should specify it's own variables instead of use shared variables.

Consider 2 root queries that are imported to the template:

query GetRecentPosts( $first: Int ) {
  posts( first: $first ) {
    nodes { 
      id 
      title
    }
  }
}
query GetMenuItems( $first: Int ) {
 menuItems( first: $first ) {
   nodes { 
     id 
     label
     parentDatabaseId
   }
 }
}

I would like to pass a different $first variable to each, and if they were receiving shared variables, then they would both get the same number of items.

This is just one example, but I could probably come up with a lot more examples where shared variables could/would lead to unexpected bugs.

I think the array of queries should look more like:

Component.queries = [
   { 
     query: GET_POST_QUERY,
     variables: ({ databaseId }, ctx) => { ... },
     possibleFutureContextToSendToWPGraphQLForThingsLikePersonalization: () => {}
   }, 
   { 
     query: GET_TEMPLATE_QUERY,
     variables: ({ databaseId }, ctx) => { ... },
     possibleFutureContextToSendToWPGraphQLForThingsLikePersonalization: () => {}
   }
]

This would allow for components to have a bit more control over their behavior at their own component level.

I like this approach. It should work with our existing implementation. I'm going to change the original RFC post to reflect support for variables.

@blakewilson
Copy link
Contributor Author

blakewilson commented Nov 1, 2023

Thanks for the clarification @blakewilson. Historically, the use of an additional hook call to resolve data has confused more people than accessing it directly via props but the hook actually seems like the clearer option in this case. I also assume that this API would be additive, meaning that Component.query would take precedence if both are defined. If that's true, this seems like a helpful addition that should help folks achieve much better cache granularity using the pages directory.

@JEverhart383 I think allowing both queries and query will be counterproductive to the end user, we'll likely allow one or the other and provide an error message to the user if both are found.

This may not be the place for it, but given all the inflight work on app router and the template hierarchy, I think it's worth calling this out here. It's clear that one large query has downsides to multiple smaller queries, but I think this has a lot to do with how the template system works currently. The frustration I hear most often from users is some variation of this question from Discord: What is the easiest way of implementing header and footer that will be shared among all pages?

Right now, most people end up reimplementing a header and footer component inside of each template, which means that its data requirements are either expressed in that template's query or passed in via props. The more I play around with RSCs, I wonder if there is a better way to organize these site parts so that my template's data requirements don't include "global" data as much as possible. When I think about traditional WP PHP templating, get_header and get_footer serve this purpose so that in most cases, my template doesn't need to concern itself with those parts and really focuses on the data and presentation that comprises the main section of content. Maybe this is a layout convention, or a pattern of colocation and an API for dynamically including site pieces based on the seed query result, and maybe I'm overthinking all of this and it just goes away with RSCs ???

I think the useFaustQuery hook is going to alleviate a lot of frustration here. We'll be using React Context here, so as long as the hook is called within a proper Faust Template, they can use this hook to retrieve a query anywhere within their components (header, footer, etc) without the need for prop drilling, which I know is a current issue with props.data.

I do think the implementation in App Router inherently looks different just because of the differences in template hierarchy between current Faust and App Router Faust. In App Router, you could have multiple React components make up a given UI (page.tsx, layout.tsx, error.tsx, etc.) and so I think when data needs to be fetched, it can be fetched as an RSC within it's respective component, and bubbling everything up into one query no longer matters so much.

@JEverhart383
Copy link

Just want to clarify this statement and maybe my original comment:

I think allowing both queries and query will be counterproductive to the end user, we'll likely allow one or the other and provide an error message to the user if both are found.

Agreed, and I can't see a reason to use both so I might have chosen a bad example, just confirming that nobody using Component.query should need to refactor anything unless they want to use additional queries.

@blakewilson
Copy link
Contributor Author

Just want to clarify this statement and maybe my original comment:

I think allowing both queries and query will be counterproductive to the end user, we'll likely allow one or the other and provide an error message to the user if both are found.

Agreed, and I can't see a reason to use both so I might have chosen a bad example, just confirming that nobody using Component.query should need to refactor anything unless they want to use additional queries.

Yes! Totally correct @JEverhart383, this will be an additive feature as you say, so no breaking changes.

@theodesp
Copy link
Member

theodesp commented Nov 2, 2023

Some notes:

  • Supporting different variables. Good!
  • Error handling scenarios. How we handle errors in requests? If one request fails should all fail?

@blakewilson
Copy link
Contributor Author

Some notes:

  • Supporting different variables. Good!
  • Error handling scenarios. How we handle errors in requests? If one request fails should all fail?

I was thinking about this as well @theodesp, but didn't include it in the RFC so I'm glad you brought this up.

Personally, I think if one query fails, they should all fail, just due to the nature of a Faust template. In my mind partial data isn't really acceptable, but there may be a use case I'm not thinking of.

Any thoughts on this? @JEverhart383 @jasonbahl

@justlevine
Copy link
Contributor

I don't know enough about RSC, so bringing this up just in case:

In current versions of Faust, the query/fragment colocation pattern often interferes with code splitting (the component can't be loaded dynamically, since the gql exists as a property on it).

Will this same issue persist with server components? Would be good to know before we literally ( 😝) double down on the pattern.

@blakewilson
Copy link
Contributor Author

I don't know enough about RSC, so bringing this up just in case:

In current versions of Faust, the query/fragment colocation pattern often interferes with code splitting (the component can't be loaded dynamically, since the gql exists as a property on it).

Will this same issue persist with server components? Would be good to know before we literally ( 😝) double down on the pattern.

Good question @justlevine, this will not persist to RSC (and app router support). Our app router support will likely not include Faust Templates as we know them today due to the nature of RSCs and how layouts work within Next.js App Router.

@dgallardox
Copy link
Contributor

I am not super familiar but I think it would be a cool optional addition. How would component.queries work using GET requests? Would the GET implementation require multiple round trips to resolve the query compared to component.query?

Also, would users be able to configure the time interval in which queries are batched together?

@theodesp
Copy link
Member

theodesp commented Nov 6, 2023

query/fragment colocation pattern

I can actually verify that query/fragment colocation pattern only works with client components used in the client side.

If you attempt to access a query/fragment client component via a server component you will get this error:

Error: Cannot access .Foo on the server. You cannot dot into a client module from a server component. You can only pass the imported name through.

vercel/next.js#51593

I'm working on a prototype implementation in this RFC to propose a structure that is compatible with RSC components

#1619

@blakewilson
Copy link
Contributor Author

I am not super familiar but I think it would be a cool optional addition. How would component.queries work using GET requests? Would the GET implementation require multiple round trips to resolve the query compared to component.query?

Also, would users be able to configure the time interval in which queries are batched together?

@dgallardox These queries would not be batched together, but be requested via a Promise.all(). We have talked about also including an apolloQueryOptions config in each Component.queries that would allow the user to modify each query as they see fit. For example, they may want their "layout" query to always use a POST request, while the rest of their queries use the default GET. They could do something like this:

export default function Component(props) {}

Component.queries = [
  {
    query: GET_POST_QUERY,
    variables: ({ databaseId }, ctx) => {
      return {
        databaseId,
        asPreview: ctx?.asPreview,
      };
    },
  },
  {
    query: GET_TEMPLATE_QUERY,
    // Variables are not required
    variables: () => {},
    apolloQueryOptions: {
      context: {
        fetchOptions: {
          method: 'POST',
        },
      },
    }
  },
];

@theodesp
Copy link
Member

theodesp commented Nov 8, 2023

Hey @blakewilson if we are using Promise.all() then we need to mention that those queries should not depend on each other since they will be send all at once.

@blakewilson
Copy link
Contributor Author

Hey @blakewilson if we are using Promise.all() then we need to mention that those queries should not depend on each other since they will be send all at once.

@theodesp 👍 Added this to the drawbacks section.

@jordanmaslyn
Copy link
Contributor

jordanmaslyn commented Nov 14, 2023

Love this RFC and the ensuing conversation.

Have we considered allowing users to specify which client to use? I know Apollo suggests that a best practice is to separate queries that are dynamic based on user from those that are global.

It would probably help cache hit performances if we allow this by letting devs specify which client to use.

This feature could also support use cases where they have a primary WP client in addition to a client for another GraphQL endpoint (e.g. Shopify).

Thoughts?

@justlevine
Copy link
Contributor

Have we considered allowing users to specify which client to use? I know Apollo suggests that a best practice is to separate queries that are dynamic based on user from those that are global.

Anything that makes Faust less coupled to its ApolloClient implementation (or makes that implementation more extendable) gets my vote

@theodesp
Copy link
Member

Since this feature is merged already in this PR
#1661

I will be closing this RFC soon.

@jasonbahl
Copy link
Contributor

I will be closing this RFC soon.

@theodesp - @josephfusco and I tried to test this out and ran into issues. We messaged @blakewilson but he was OOO so waiting to chat with him to get more insight and hopefully get some feedback on the implementation

@theodesp
Copy link
Member

@jasonbahl thanks. I will keep it open then until those issues are resolved.

@jasonbahl
Copy link
Contributor

What happens if the same query is used with different variables?

Let's say I had a template that had various sections of posts (like a newspaper homepage)

And I had several components that fetched a list of posts.

And I had the following queries on my template:

i.e.

HomePageTemplate.queries = [
  {
     query: GET_POSTS,
     variables: { 
        category: "featured"
     },
     query: GET_POSTS,
     variables: { 
        category: "sports"
     },
     query: GET_POSTS,
     variables: { 
        category: "news"
     },
  }
];

If the data is hashed on the query alone, then all these sections would have the same cached results.

I could see this happen for templates that have multiple nav menus too.

Like what if I had:

const HomePageTemplate = () => {
  return (
    <>
       <HeaderMenu />
         <Content />
       <FooterMenu />
    </>
  )
}

HomePageTemplate.queries = [
  {
    query: NAV_MENU_QUERY,
    variables: { menu: "primary" }
  },
  {
    query: FOOTER_MENU_QUERY,
    variables: { menu: "footer" }
  },
  {
    query: CONTENT_QUERY,
    variables: ({uri}) => ({uri})
  }
]

Both the Header and Footer menu would have the same data returned from useFaustQuery because they'd be hashed against the NAV_MENU_QUERY, not considering the variables

@jasonbahl
Copy link
Contributor

jasonbahl commented Jan 12, 2024

@blakewilson I think similar to what I suggested here: #1628 (comment)

Each query might have different params associated with it that make it unique, and perhaps the cache key should be a hash of the entire object that makes a query a query.

So instead of a hash of just the query, it would be a hash of the Query, Variables and any other keys passed to the object.

i.e.

hash( { query: MY_QUERY, variables: { ... }, myFuturePersonalizationIdentifier: { ...}, extensions: { locale: "en_US" } )

etc

Then for useFaustQuery instead of just the query, it might be something like:

useFaustQuery( {
  { 
    query: MY_QUERY, variables: { ... }, 
    myFuturePersonalizationIdentifier: { ...}, 
    extensions: { locale: "en_US" }
  } 
)

🤷🏻‍♂️

@blakewilson
Copy link
Contributor Author

@jasonbahl Thanks for your feedback on the support for multiple of the same query. I've added your comments to the dedicated issue here:

#1742

@blakewilson
Copy link
Contributor Author

Since this feature has been shipped, I'm going to close this issue. Thanks everyone for your feedback!

If you have further ideas regarding multiple queries in Faust, please open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants