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

Generate Prisma Client when prerendering #6093

Merged
merged 6 commits into from
Aug 5, 2022

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jul 31, 2022

After #5600 we now need access to the Prisma Client when pre-rendering. So even when just building for the web side we still need to generate the client if we're going to pre-render.

Just enabling the step for generating the client wasn't enough however. A new process needs to be kicked off for pre-rendering in order for it to pick up the new client.

Fixes #6088

@nx-cloud
Copy link

nx-cloud bot commented Jul 31, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 30d9e54. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jul 31, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 30d9e54
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62ed1a919c97390008a0b1aa

@dac09
Copy link
Contributor

dac09 commented Aug 1, 2022

So we always need access to Prisma or just when accessing db in routeHooks? Because this might point to a bigger issue (just in CLI imports, if I had to guess)

It might make more sense to move the prisma generate step to the prerender CLI so captures more cases? Otherwise it'll fail when you run yarn rw prerender and it won't make sense why.

@Tobbe
Copy link
Member Author

Tobbe commented Aug 1, 2022

So we always need access to Prisma or just when accessing db in routeHooks? Because this might point to a bigger issue (just in CLI imports, if I had to guess)

We instantiate the user's graphql handler, and it uses db. So we always need it.

import { createGraphQLHandler } from '@redwoodjs/graphql-server'

import directives from 'src/directives/**/*.{js,ts}'
import sdls from 'src/graphql/**/*.sdl.{js,ts}'
import { getCurrentUser } from 'src/lib/auth'
import { db } from 'src/lib/db'
import { logger } from 'src/lib/logger'
import services from 'src/services/**/*.{js,ts}'

export const handler = createGraphQLHandler({
  getCurrentUser,
  loggerConfig: { logger, options: {} },
  directives,
  sdls,
  services,
  onException: () => {
    // Disconnect from your database with an unhandled exception.
    db.$disconnect()
  },
})

@dac09
Copy link
Contributor

dac09 commented Aug 2, 2022

@Tobbe I think I know why you need to run it as a separate process - just a thread to pull on.

Node will cache any imports (require statements) it sees in the run path - so when we first run the build command, it probably loads the graphql handler, and src/lib/db, before Prisma has had a chance to generate. This now gets cached.

So even though in later steps you generate the Prisma client, because the module was already loaded, it doesn't change.

It's worth making sure the imports of the graphql handler are done async (I haven't checked the code here though)

@Tobbe
Copy link
Member Author

Tobbe commented Aug 2, 2022

@dac09 Thanks for spending some time thinking about this.

I'll see what I can come up with between your idea of moving prisma client generation to the prerender cli, and those thoughts about node caching the imports

@Tobbe
Copy link
Member Author

Tobbe commented Aug 3, 2022

It's worth making sure the imports of the graphql handler are done async (I haven't checked the code here though)

It it async already, so that wasn't the easy fix we were hoping for 🙂

export async function getGqlHandler() {
  const gqlPath = path.join(getPaths().api.functions, 'graphql')

  const { handler } = await import(gqlPath)

  return async (operation: Record<string, unknown>) => {
    return await handler(buildApiEvent(operation), buildContext())
  }
}

@Tobbe Tobbe mentioned this pull request Aug 3, 2022
18 tasks
@Tobbe
Copy link
Member Author

Tobbe commented Aug 3, 2022

It might make more sense to move the prisma generate step to the prerender CLI so captures more cases? Otherwise it'll fail when you run yarn rw prerender and it won't make sense why.

Trying to implement this I realized two things:

  1. We already require our users to run yarn rw build web at least once before running prerender. We print an error message and quit if they haven't. So as long as prisma generate is part of build web we should still be good
  2. Making prisma generation part of prerender makes it more difficult again to run them in separate processes. So running into that same error message I had from the start.

So either we keep it the way it is in this PR. Or we'll have to figure out how to generate the prisma client and run prerender in the same process

Copy link
Contributor

@dac09 dac09 left a comment

Choose a reason for hiding this comment

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

Lets get this in... but I still can't reproduce the following.

A new process needs to be kicked off for pre-rendering in order for it to pick up the new client.

@dac09 dac09 enabled auto-merge (squash) August 4, 2022 17:35
@Tobbe Tobbe disabled auto-merge August 4, 2022 18:39
@Tobbe Tobbe force-pushed the tobbe-generate-prisma-web branch from ca384b3 to e954221 Compare August 5, 2022 05:16
@Tobbe
Copy link
Member Author

Tobbe commented Aug 5, 2022

@dac09 I figured out how to get it working without running in a separate process.
The key is this

// Purge Prisma Client from node's require cache, so that the newly
// generated client gets picked up by any script that uses it
Object.keys(require.cache).forEach((key) => {
  if (
    key.includes('/node_modules/@prisma/client/') ||
    key.includes('/node_modules/.prisma/client/')
  ) {
    delete require.cache[key]
  }
})

Which solution do you prefer? Deleting from the cache like that, or spinning up a separate process?

@dac09
Copy link
Contributor

dac09 commented Aug 5, 2022

I think spinning up a separate process - just feels like there'll be less edgecases. But I'm glad we know what the cause is!

@dac09
Copy link
Contributor

dac09 commented Aug 5, 2022

But I'm just going by gut feel - ideally we'd make sure prisma doesn't get imported - because that's likely to have memory benefits.

Copy link
Member Author

Tobbe commented Aug 5, 2022

I think in most real-world scenarios prisma will be needed. Fetching ids from you DB is probably going to be the most commons scenario I think

@dac09
Copy link
Contributor

dac09 commented Aug 5, 2022

It doesn’t need to be imported before generating the prisma client

@Tobbe Tobbe enabled auto-merge (squash) August 5, 2022 13:27
@Tobbe Tobbe merged commit 5ebd4e8 into redwoodjs:main Aug 5, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Aug 5, 2022
@Tobbe Tobbe deleted the tobbe-generate-prisma-web branch August 6, 2022 09:19
@jtoar jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug?]: Deploy to Netlify and Vercel not working when prerendering with Redwood canary
3 participants