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

Client RPC Transformation #88

Closed
flybayer opened this issue Apr 7, 2020 · 12 comments
Closed

Client RPC Transformation #88

flybayer opened this issue Apr 7, 2020 · 12 comments
Assignees

Comments

@flybayer
Copy link
Member

flybayer commented Apr 7, 2020

I'm sure there are many ways we could do this, but here's what I'm thinking:

Overview

Essentially, every usage of a query or mutation inside a React component anywhere in the project should be replaced with a fetch call.

  • Blitz exports an RPC client
  • During build time, the direct query/mutation call is replaced with a call to the rpc client.

App code:

import getProduct from 'app/product/queries/getProduct'

const product = await getProduct({where: {id: 1}})

Built code:

import {rpc} from 'blitz'

const product = await rpc('/api/product/queries/getProduct')({where: {id: 1}})

But query/mutation usage in server side code should not use RPC. It should still use the query/mutation directly

App code:

import {useQuery} from 'blitz'
import getProduct from 'app/product/queries/getProduct'

export const getStaticProps = async context => {
  const product = await getProduct({where: {id: context.params?.id}})
  return {props: {product}}
}

export default function(props) {
  const [product2] = useQuery(getProduct, {where: {id: props.id})
  return (
    <div>
      <h1>{props.product.name}</h1>
    </div>
  )
}

Built code:

import {useQuery, rpc} from 'blitz'
import getProduct from 'app/product/queries/getProduct'

export const getStaticProps = async context => {
  const product = await getProduct({where: {id: context.params?.id}})
  return {props: {product}}
}

export default function(props) {
  const [product2] = useQuery(rpc('/api/product/queries/getProduct'), {where: {id: props.id})
  return (
    <div>
      <h1>{props.product.name}</h1>
    </div>
  )
}

Implementation

I think this is a pretty straightforward babel transformation, right? The only part I'm not sure about is how we can know the file path to the query/mutation from inside the babel transformation. Maybe we first need to compile a manifest file of all queries/mutations that the babel transform can read from.

@ryardley ryardley self-assigned this Apr 7, 2020
@ryardley
Copy link
Collaborator

ryardley commented Apr 7, 2020

I think it might be easiest to look for usage of useQuery and then work back using references. That way we shouldn't need to distinguish between serverside and clientside. The rpc module should know how to find the correct endpoint. It should only need the import path and possibly the named export if we want to allow named exports.

@flybayer
Copy link
Member Author

flybayer commented Apr 8, 2020

We can't rely on useQuery usage, because you should be able to manually use queries and mutations without useQuery. useQuery is only there as a declarative hook interface.

@ryardley
Copy link
Collaborator

ryardley commented Apr 8, 2020

Hmm ok. A couple of thoughts. How do we warm the lambda for a mutation if all we are doing is importing it and running it in a callback? Seems to me currently useQuery is acting mainly as a proxy for React Query query caching. What about return values from mutations? Surely they need to invalidate the cache key? Could accessing mutations somehow through a hook solve that problem as you would have access to the component render cycle?

Perhaps it is a mistake to try to create and maintain a complex babel transform. One idea would be to build an rpc client that can be run on both the client and the server and based on its environment will either make the HTTP call or directly call the resource. That way the only transformation required would be to change the import path from "app/product/queries/getProduct" to a pre-rendered isomorphic rpc client at "_rpc/app/product/queries/getProduct" or something similar. That might make for less confusing babel code? I attempted to look at some old babel transform code I did the other day and... man if stuff gets complex it is not much fun. Also that way if you ever wanted to speed it up and you had no more transformations you could do it with a regex and avoid a babel transform altogether.

@flybayer
Copy link
Member Author

flybayer commented Apr 8, 2020

Warming the Lambda

Oops, I forgot this! We can do that like shown below.

App code:

import getProduct from 'app/product/queries/getProduct'

const product = await getProduct({where: {id: 1}})

Built code:

import {rpc} from 'blitz'

const warmedRpc = rpc('/api/product/queries/getProduct') //makes HEAD request
const product = await warmedRpc({where: {id: 1}})

Query cache invalidation

Great point. This needs more attention. I'll open another issue on useQuery, mutations, and cache invalidation.

Once we figure that out, then we can come back here and figure out the best way to handle it under the hood.

Babel transform?

Perhaps it is a mistake to try to create and maintain a complex babel transform. One idea would be to build an rpc client that can be run on both the client and the server and based on its environment will either make the HTTP call or directly call the resource.

I'm totally fine with this approach! We just have to ensure the server code isn't bundled with the client.

@flybayer
Copy link
Member Author

flybayer commented Apr 8, 2020

Alright, here's the issue on query/mutation usage: #89

@flybayer
Copy link
Member Author

flybayer commented Apr 9, 2020

@ryardley what are you thinking by now on how to approach this?

@ryardley
Copy link
Collaborator

ryardley commented Apr 9, 2020

I think as far as this transformation task is concerned I think your idea to warm the lambda by transforming every querymutation to be a call to rpc with the absolute import path is great. It might make for babel transform complexities though this could be mitigated during query compilation. I can see advantages to the idea around simply swapping out the path universally with a universal rpc client but then calling the result as a function in place as you only need a single AST traversal even though there will be a couple more transfomations. Would have to test but this might actually be both more efficient and simpler babel code. If we can do everything in a single traversal that would be ideal.

// orig
import getProduct from 'app/product/queries/getProduct';

export async function getServerSideProps() {
  const product = await getProduct({where: {id: 1}})
  // ...
}

const product = await getProduct({where: {id: 1}})

becomes

import getProduct from '_rpc/app/product/queries/getProduct';

export async function getServerSideProps() {
  const product = await getProduct()({where: {id: 1}})
  // ...
}

const product = await getProduct()({where: {id: 1}})

vs

import getProduct from 'app/product/queries/getProduct'
import {rpc} from 'blitz'

export async function getServerSideProps() {
  const product = await getProduct({where: {id: 1}})
  // ...
}

const product = await rpc('app/product/queries/getProduct')({where: {id: 1}})

@flybayer
Copy link
Member Author

flybayer commented Apr 9, 2020

Yeah, that looks promising! I've never actually written a babel transformation, so I trust your judgement on this!

@flybayer
Copy link
Member Author

I had a realization: Next does two builds: server bundle and client bundle. So I think we only have to solve each separately. We don't have to solve both at once.

Therefore I think we can do this without any transform at all.

Server bundle

  • Leave the original code as is. No changes

Client bundle

  • Next removes gSSP from this build, so it's irrelevant.
  • We use webpack to alias query import to the query rpc import
  • For warming Lambda, we don't need to transform useQuery(getProduct, args) to useQuery(getProduct(), args). Instead we make the HEAD call inside the rpc client as a side effect of the es6 import
// orig & server bundle
import getProduct from 'app/product/queries/getProduct';

export async function getServerSideProps({req, res}) {
  const product = await ssrQuery(getProduct, {where: {id: 1}})
  // ...
}

function Page() {
  const product = useQuery(getProduct, {where: {id: 1}})
  // ...

for the client bundle, the only change is a webpack alias for the import:

// client bundle
import getProduct from 'app/product/queries/getProduct'; // aliased to '_rpc/something`

function Page() {
  const product = useQuery(getProduct, {where: {id: 1}})
  // ...

@ryardley
Copy link
Collaborator

Interesting hack. Really like the idea to just use a webpack alias. I was thinking about that but wrote it off as it might fire off all lambdas on initial page load. However now you bring it up I realise I forgot about code splitting which might save us as only lambdas first defined in the page will be warmed on initial page load and the rest as soon as each subsequent code splitting bundle is loaded. If the app is the kind that is used for a long time lambdas might go to sleep unless we add a keep alive. So we will want to have a setInterval for a minute to run that head call again. If we are doing that would we want to just warm up all lambdas at staggered intervals on page load? The whole thing seems clumsy and blunt though with wasted server client traffic.

@flybayer
Copy link
Member Author

Let's just do one initial warm for now and see how it goes. It's impossible to guarantee you won't hit a cold lambda. Best we can do is minimize it to some extent. I think cold starts will be most noticeable for very low traffic apps where there's only 1 person using the app at a time. In this case, warming, at initial load, the lambdas that are likely to be used next will help to some extent.

@flybayer
Copy link
Member Author

Closed by #95

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

No branches or pull requests

2 participants