HoudiniGraphql / houdini

The disappearing GraphQL client
http://www.houdinigraphql.com
MIT License
893 stars 94 forks source link

Rethink non-Route parameter Variable API #1211

Open AlecAivazis opened 10 months ago

AlecAivazis commented 10 months ago

Describe the feature

Right now, if a user wants to provide data for a query that defined inside of +page.gql then they currently have 2 options:

The unfortunate reality is that not all parameters can be naturally encoded in the URL. Take for example Vercel's profile switcher. Since the value for which org is selected is not encoded in the URL, the use has to split their data-fetching logic across 2 files:

+page.gql

query OrganizationInfo($organization: ID!) {
    organization(id: $organization) {
        projects { 
            id
        }
    }
}

+page.js(x)

export const _OrganizationInfoVariables = ({session}) => {
    return { 
        organization: session.organization
    }
}

This feels very disconnected. I'd like to see if we can come up with a solution that feels more streamlined with this approach being as last-ditch escape hatch to wire up query values.

Obviously the biggest constraint in the API is how to clearly map query variables to a javascript function. The current solution does this by looking for a magical function. I have a few ideas for ways to pull this off

~Dynamic~ Runtime Scalar

One idea I had was to define something like a custom scalar (ie, OrganizationFromSession that you can configure to be equivalent to a String but with a value that's computed at runtime:

query MyQuery(organization: OrganizationFromSession!) {
    organization(id: $organization) {
        ...
    }
}
export default {
    // ...
    scalars: {                                 // maybe a different key ... RuntimeScalars??
        OrganizationFromSession: {
             graphqlType: "ID",
             value: ({ session }) => session.organization
        }
    }
}

we already enforce that the config file needs to be importable by the server and client so i think we can pretty safely leave it up to the user to make sure the body of that function always works (ie, it can't just always read from local storage)

This API works really well for values that are core to an application-specifics. It probably only feels good for values that are global (ie in the session).

pros:

cons:

Yet-Another-Directive

I think this one is slightly more beginner friendly:

query MyQuery(id: StringFromSession) @variableValue(fn: "src/variables/MyQuery") {
    user(id: $id) {
        ...
    }
}

Pros:

Cons:

Criticality

cool improvement, my projects will benefit from it

cc @pixelmund @jycouet

jycouet commented 10 months ago

Yes, exploring new avenues is a good idea. Could this issue also be considered under the Rethink for Yet-Another-Directive's checkSearchParam?

About Dynamic Scalar

I really like this concept because it's expressive, readable, and aligned with the GraphQL mindset. It might be a bit challenging for newcomers, but perhaps it's our job to make it beginner-friendly πŸ’ͺ. One downside is that you can't directly copy and paste the +page.gql content into a GraphiQL playground to test queries. This is due to the presence of CustomScalars. Maybe Houdini could introduce a customized GraphiQL that understands these nuances and interacts appropriately with the final endpoint? πŸ€”

About Yet-Another-Directive (@variableValue)

In my opinion, this doesn't significantly improve the current approach. At present, we instruct users that _variables are all they need to focus on. With this new directive, they'll have to consider a file path, maintain consistency across projects, and manage default exports (or enforce function names or work around string properties). On the flip side, you can point to '/src/variables/getOffsetFormSearchParam' and reuse it across various components. πŸ‘

About Mixed Cases

What if we have mixed scenarios, like id coming from /routes/[id], offset coming from searchParma URL, and info coming from the session?

query ContractOrders($id: ID!, $offset: Int, $info: String) {
  contract(id: $id) {
    id
    name
    orders(offset: $offset, info: $info) {
      id
      quantity
    }
  }
}

# Using Dynamic Scalar
query ContractOrders($id: ID!, $offset: OffsetFromSearchParam, $info: StringFromSession) {
}

# Using Yet-Another-Directive (@variableValue)
query ContractOrders($id: ID!, $offset: Int, $info: String) @variableValue(fn: "/src/variables/getOffsetFormSearchParam") {
}

# Using Yet-Another-Directive (@checkSearchParam) (@checkSession would look somewhere a global transform?!)
query ContractOrders($id: ID!, $offset: Int, $info: String) @checkSearchParam @checkSession {
}

What are the ideal trade-offs? What is most commonly used? (Mixed? param only? session only? query string only? ...)

I'm eager to hear comments and ideas πŸ˜‰

AlecAivazis commented 10 months ago

Looking at all of the options laid out like this, I think the first option is the cleanest. The biggest win in my eyes is the natural ability to compose the different kinds of variables together. The distinction is even more clear if a user is willing to impose a naming convention on these kinds of scalars (another lint rule?)

We could pull off something similar if we allowed for @variableValue to fall on an argument definition but i dont think its worth it:

# Using Yet-Another-Directive (@variableValue)
query ContractOrders($id: ID!, $offset: Int @variableValue(fn: "/src/variables/offsetFromSearchParam"), $info: String) {

Beyond just the noise, there's also some weird coupling here for the generated types since the function has to match the nullability of the field and the same function could be used by multiple fields with different nullabilities.

On the flip side, you can point to '/src/variables/getOffsetFormSearchParam' and reuse it across various components.

I think this is the most compelling reason for @variableValue but it can be accomplished with "Dynamic Scalars" if the user imports the shared function into their config file.

Maybe Houdini could introduce a customized GraphiQL that understands these nuances and interacts appropriately with the final endpoint?

Yea! I like this a lot. At the very least we should see if there is a plugin interface we can hook to. that would save us from having to maintain an entire graphiql component for all of the different frameworks.

Could https://github.com/HoudiniGraphql/houdini/issues/1210 also be considered under the Rethink for Yet-Another-Directive's checkSearchParam?

I think these two issues are distinct and the solution for one might not apply to the other situation. For example, my gut says that driving search parameters with a directive might be a good idea but it doesn't feel like the right fit for non-route parameters.

AlecAivazis commented 10 months ago

One con that occured to me today with the scalar approach is that you can only set one value at a time

jycouet commented 10 months ago

One con that occured to me today with the scalar approach is that you can only set one value at a time

It's a con & pro, being very expressive can be good as well.

query ContractOrders($id: Param, $limit: SearchParam, $offset: SearchParam, $info: Session) {}
AlecAivazis commented 10 months ago

query ContractOrders($id: Param, $limit: SearchParam, $offset: SearchParam, $info: Session) {}

That wouldn't quite work - you wouldn't know if you are computing the limit or offset from the search param. How would you know which value to take from the url? Maybe we need to give the runtime signature the name of the field?

jycouet commented 10 months ago

I'm thinking black magic here ^^ As a first param to SearchParam, we could send the name of the variable?

scalars: {                     
  SearchParam: {
    graphqlType: "String",
    value: (variableName, { url }) => url.searchParam.get("variableName")
  }
}

I have no clue if/how it's possible yet. But we can probably explore?

endigma commented 7 months ago

Could the function for resolving get the same inputs as a load ? I also don't think mixing this into scalars is a great idea, wouldn't that mangle this clientside magic with server-client type bindings?

AlecAivazis commented 7 months ago

Not sure I understand the concern about type bindings - could you give me an example? My original thinking was that the graphqlType field would be used to ensure the proper types are being generated.

endigma commented 7 months ago

I just mean treating it as a server-side type like a scalar in the config is a little mixed isn't it? Putting it in scalars with all of this server->client translation stuff: image

SeppahBaws commented 7 months ago

I stumbled upon this issue again after a while, and finally have some time to write out some thoughts.

(I assume that this issue is talking about component queries)

About dynamic scalars

These scalars are provided by Houdini and thus don't exist in the schema on the server, making it annoying to work with in external graphql IDEs. There is a point to be made about creating a plugin of sorts for a custom GraphiQL experience, but there are other GQL IDEs available out there (I personally prefer to use Hot Chocolate's Banana Cake Pop)

In the case of component queries, it would be impossible to pass parameters passed to the component all the way to the client.ts file.

I do think that the dynamic scalars could be interesting for more global variables - like the session key provided above - but I don't believe that this could replace the existing magic _<my-query>Veriables function.

About another directive

At first glance this has the same issue with it not existing on the schema. To add a bit to that, the GraphQL plugin for vs code isn't really great at the moment for svelte, and more than not fails to properly detect the types and cannot properly do auto-complete. It sometimes already is a bit annoying to write the parameters for @with directives without autocomplete (since they are magically detected for the queries), so adding yet another directive might not be the most fun experience

Additionally, having to point to a variable function where the proper value gets passed along sounds very tedious. It also diminishes the point of colocating related things, something that I think is a huge benefit of Houdini with the way it handles +page.gql files etc.

Maybe this?

I do agree that the magic _<my-query>Variables function isn't the greatest experience, as I think it can be a little bit too much black magic. Having to export it for a component query to work is also a bit ugly, as it then "leaks" into the autocomplete of anyone who wants to use that component.

Purely from the perspective of a component query, maybe we can combine the dynamic scalars with a different approach to passing in query params?

<script lang="ts">
  export let userId: string;

  $: store = graphql(`
    query MyQuery($id: ID!, $session: StringFromSession!) @load {
      user(id: $id, session: $session) {
        name
      }
    }
  `, {
    variables: {
      id: userId
    }
    // Maybe this space could even be used to pass in other options
    // like fetch policy etc
  });
</script>

This way we don't have to export a black magic _<my-query>Variables function and can keep the benefits from having the dynamic scalar whose value gets provided globally.

What would need some investigating is what would happen if one of the component props gets updated, as that would require a refetch of the component. (maybe we could even improve this, as with the old _<my-query>Variables would cause the component to trigger a refetch, even if a prop was updated that doesn't get passed as variable to the query)

AlecAivazis commented 7 months ago

Wow thanks for taking the time to compile your thoughts! I was looking to revamp the entire query variable API but component queries are definitely one of the places the magic function feels the worst.

So historically, the reason we have avoided the extra argument to the graphql function was because it would confuse users who have defined their queries in their +page.svelte files. In retrospect, I probably should have removed that API when we transitioned to 1.0 but it is what it is - something we can consider for houdini 2.0. Anyway, the issue is that that value isn't easily transferable to a load function inside of +page.js using static tricks since expressions could have local variables or import third party modules.

fwiw, I'm also open to relaxing the need for export in components.

tommyminds commented 6 months ago

While the magic functions may seem most troublesome for component queries, losing the ability to use +page.gql in conjunction with search parameters as query variables is definitely not ideal.

Requiring a +page.js file with _houdini_load and _MyStoreVariables feels like a significant step down from simply having a +page.gql.

Could it perhaps be possible to have a +page.gql file containing the query document that is automatically loaded, while having a +page.js file containing an extractVariables function that overrides the default route-based variable strategy?

AlecAivazis commented 5 months ago

Could it perhaps be possible to have a +page.gql file containing the query document that is automatically loaded, while having a +page.js file containing an extractVariables function that overrides the default route-based variable strategy?

Assuming i'm following you correctly @tommyminds, I think this is pretty much what we have now. Users have to define a function in +page.js to pull the values out. I'm not a huge fan of spread out it feels. I think it's an alright "low-level" escape hatch but I'm hoping we can come up with a solution that feel as streamlined as +page.gql.

endigma commented 5 months ago

I think the thinking for streamlining should probably start at where other than routes a user might get a variable from:

Maybe there can be some directive to "fill" a variable from various sources? This could be a leaky if its resolved clientside so it should be a generated directive, but it could be like @fill(field: string, value: string) where value is some dot-accessor to the available objects?

AlecAivazis commented 5 months ago

Funny you should mention that @endigma - i was thinking something very similar last night but wasn't quite sure how to dress up the API. I think the core insight in this is that the inputs of this function are pretty much fixed to things that are accessible on the server. I think the biggest hurdle will be to validate the dot notation, although maybe its just an acceptable compromise that we don't know the shape of your Session (without doing some typescript introspection that seems too hard to get right to be worth it)

AlecAivazis commented 5 months ago

Another thought I had is to restructure the current Variables api to be less noisy, something like:

import { QueryVariables } from './$houdini'

export const _variables: QueryVariables = ({ session } ) => {
    return {
        OrganizationInfo: { 
            variable: session.organization
        }
    }
}

As a low-level API this feels slightly better since there's no need to worry about the overlapping type and function name and its more clear that you can pass values for each query available to your route. This comes at the cost of not being able to catch that they are defining functions for every query that needs it. For example, if there are multiple queries that need variables, we would only know they are passing values for a specific query by analyzing the return structure of the variable function and that can be pretty brittle - users like to do crazy things in functions when they can.