Arbitrary query variables

[vgedzius]

Hi, current version of library only passes variables from the route. This pull request is for changes that allow passing arbitrary variables to the query. Haven't defined proper variable types, maybe someone with deeper typescript skills can have a crack at it, other than that should be pretty straight forward change

[rrdelaney]

This is great, thank you so much for your contribution!

I've been thinking about this for a bit too, one of my thoughts is that this will run in a similar fashion to getInitialProps, in that it will run on both the client-side and server-side, which can cause a query to have different variables at runtime. In the case of our app most of the query variables we want are stored in a combination of pageProps and React context, it would be very cool if we could extract query variables out of those.

Will think a bit more about this over the course of the day, in the meantime could you add usage of this to the example project? Thanks again!

[vgedzius]

Added example to the docs.

[rrdelaney]

I've thought about the API a bit more, and I'm not sure that adding a new config option is the best way to go because it would run on a the server and client. What do you think about merging the results of getServerSideProps or getClientSideProps into the query variables? I think it serves the same use-case but has a better distinction of where the code is running.

[vgedzius]

Yeah, that is a fair point. How about having separate functions for server and client side? Something like serverQueryVariables and clientQueryVariables. If needed results of getServerSideProps and getClientSideProps can be passed as arguments for the variable functions. I feel like merging those directly into query variables could lead to some unintended consequences. Passing them through dedicated functions will give more control over what we are querying.

On a side note, how do you test this package locally? Tried npm run build and then npm link, to bring it to my own project, but getting errors that can't resolve 'react'. Perhaps a short section in readme would be helpful for future contributors.

[rrdelaney]

Thought a bit about this, I think having the two separate functions that get the ctx and the respective generated props works well. Would be happy to merge that solution. As far as testing I manually re-write the imports in the example directory.

[mjnaderi]

Thanks for this PR, I do really need this feature. @rrdelaney Is this going to be merged soon?

I think current implementation can be improved to be more flexible.

I suggest modifying this:

  const variables = opts.serverSideQueryVariables
    ? opts.serverSideQueryVariables(ctx, serverSideProps)
    : {};
  const preloadedQuery = loadQuery(env, query, {
    ...ctx.query,
    ...variables,
  });

to this:

  const variables = opts.serverSideQueryVariables
    ? opts.serverSideQueryVariables(ctx, serverSideProps)
    : ctx.query;
  const preloadedQuery = loadQuery(env, query, variables);

(and the same for clientSideQueryVariables)

This is more flexible, and with serverSideQueryVariables = (ctx, serverSideProps) => {...ctx.query, ...someOtherVariables} it has the same effect of the first code.

The first code merges ctx.query into variables and developer has no control on it. But in second code, developer controls how to merge ctx.query into variables.

In my use case, I want to apply a transformation on ctx.query before passing it to variables. For example if we have {is_active: "on", page: 4} in ctx.query, I want to pass {is_active: true, first: 20, offset: 60} as GraphQL variables. I don't want page to appear in variables.

[rrdelaney]

@mjnaderi the transform approach sounds good to me. Would happy happy to accept a PR with that implemented. I'm not sure if this one is completely updated to the client / server split.

[rrdelaney]

Would you need to modify the variables based on the props? That's the only thing giving me a little hesitation here. If it was a pure function on the Next.js context that'd be a little simpler. IMO I don't see a reason to change based on props because your server can change the response instead. Any async work here is also going to slow the page down a lot.

[mjnaderi]

@rrdelaney No I don't need to modify based on props. I just need to modify variables based on ctx.query in a synchronous function.

[rrdelaney]

I like that a lot. When I was making the API I figured it should be stateless based on route and anything else passed as headers in the network config.

[vgedzius]

Sorry guys, I ended up writing my own relay integration for our project, but feel free to pick this one up from here.

[rrdelaney]

This was implemented in 92d54f6ad5b5f871acdea5a38382ca9c32966e6f with the introduction of the variablesFromContext option.