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

Update Prisma to v5 #2231

Merged
merged 19 commits into from
Sep 4, 2024
Merged

Update Prisma to v5 #2231

merged 19 commits into from
Sep 4, 2024

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Aug 8, 2024

Closes #1281
Closes #1570
Closes #2099

Left to do

  • [Blocker 🔴] Make sure CRUD features works again (Prisma types are not compatible with SuperJSON types for some reason)
  • Update e2e tests
  • Update docs No updates needed
  • Update example apps
  • Update starters
  • Format the outputted schema.prisma to enable checking if users ran the npx prisma command manually
  • Test with the waspc/examples/todoApp
  • Test with a new basic app
  • Test with embeddings app (uses preview features)
    • Works well, still need to update the starters template to have Prisma version in package.json as 5.18.0
  • Update to latest Prisma version

@@ -103,7 +103,12 @@ export type CreateActionResolved = typeof _waspCreateAction

{=# crud.operations.Update =}
{=^ overrides.Update.isDefined =}
type UpdateInput = Prisma.{= crud.entityUpper =}UpdateInput & Prisma.{= crud.entityUpper =}WhereUniqueInput
type UpdateInput = Prisma.XOR<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes UpdateInput to match what Prisma Client expects.

@@ -0,0 +1,14 @@
// Without this import, Prisma types are resolved incorrectly.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sodic this is the fix I implemented in the end, it slightly nudges the FieldRef type to work with our SuperJSON serialization.

I went down the rabbit hole of recursively converting all interfaces into types using Pick<T, keyof T> (as suggested by some in the Github issue) but this ended up being less problematic and simple enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code comment doesn't fully explain what's happening and what the tradeoffs are. Do you mind adding this playground link too.

It currently implies that type types have an implicit index signature (which isn't really true, they only exhibit special subtyping behavior). The comment also doesn't mention the compromise we made by declaring an extension to the FieldRef interface.

Anyway, both things should be clear from the playground, and you can adjust it as you see fit.

BTW, what happend with trying to fix the root of the problem, the definition of the JSONSerializable type (as attempted here). Did Superjson make it too complex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the comment to mention the playground to make it more complete as an explanation.

BTW, what happend with trying to fix the root of the problem, the definition of the JSONSerializable type (as attempted here). Did Superjson make it too complex?

We concluded that JSONSerializable would stop users from using the Prisma field reference feature: https://www.prisma.io/docs/orm/reference/prisma-client-reference#compare-columns-in-the-same-table since the proposed approach drops fields.

Then we set on to recursively convert all interfaces to types - which in the end was a long way of doing this d.ts patch we have in this repo.

@@ -91,7 +91,10 @@ export type GetQueryResolved = typeof _waspGetQuery

{=# crud.operations.Create =}
{=^ overrides.Create.isDefined =}
type CreateInput = Prisma.{= crud.entityUpper =}CreateInput
type CreateInput = Prisma.XOR<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes CreateInput to match what Prisma Client expects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I trust this is correct, but can you give more details?

Asking just out of curiosity.

Copy link
Contributor Author

@infomiho infomiho Sep 4, 2024

Choose a reason for hiding this comment

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

I copied the type the create method has defined for its data input. Before I only used the simpler type because I thought it was enough, but now I realise it's better to follow 1:1 what Prisma uses internally - users will get the same type as if they were using create directly themselves.

@@ -39,9 +50,7 @@ const entities = {
// 1. We either use the default implementation of the operation...
=}
{=^ overrides.GetAll.isDefined =}
type GetAllInput = {}
Copy link
Contributor Author

@infomiho infomiho Aug 14, 2024

Choose a reason for hiding this comment

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

For some reason, we built these types in two places: in the server and the in the SDK, but there was no need since the server can import the built types from the SDK. This change ensures that CRUD operation types are coming one a single source of truth.

@@ -1,5 +1,5 @@
datasource db {
provider = "sqlite"
provider = "postgresql"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mistake that was left after the migration to schema.prisma file. I'm not sure how that got through.

Equal<
typeof taskToTaskUnspecified,
(args: Task) => ReturnType<typeof taskToTaskUnspecifiedDefinition>
Expect<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests now pass 👍

@infomiho infomiho marked this pull request as ready for review August 14, 2024 11:44
@@ -5,6 +5,6 @@ export const printTimeAndNumberOfTasks: PrintTimeAndNumberOfTasks<
{},
void
> = async (data, context) => {
const count = await getTotalNumberOfTasks(null, context)
const count = await getTotalNumberOfTasks()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This broke after the last operations API update, quick fix.

Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
@infomiho infomiho force-pushed the miho-prisma-5 branch 2 times, most recently from 70656ad to 7ddd6a7 Compare August 20, 2024 15:39
@infomiho infomiho requested a review from sodic August 22, 2024 14:16
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Cool stuff @infomiho ! I will leave it to @sodic to check the Typescript stuff. The rest is looking good to me.
I assume you did a "diff" between the Prisma version we were on and Prisma 5, by checking their Changelog -> is there anything new we should be adding support for in Wasp, or taht significantly changed? Seems from this PR that not much changed on the API level?

Copy link
Member

Choose a reason for hiding this comment

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

@infomiho you tested all these apps with the new Prisma? They all work fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, all the apps were tested and they worked fine. The v5 major version was more about the new JSON protocol than anything, so nothing broke in our apps really :)

@@ -143,11 +143,7 @@ setDefaultCliEnvVars = do
where
cliEnvVars :: [(String, String)]
cliEnvVars =
[ ("PRISMA_HIDE_UPDATE_MESSAGE", "true"),
-- NOTE: We were getting errors from Prisma v4 related to their Checkpoint system
Copy link
Member

Choose a reason for hiding this comment

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

Ok sweet!

-- | One of the checks we perform is to compare the Wasp generated schema.prisma file
-- and the schema.prisma file in the node_modules. Prisma formats the schema in node_modules
-- automatically, so we have to do the same to be able to compare them.
formatPrismaSchemaFileOnDisk :: Path' Abs (Dir ProjectRootDir) -> IO ()
Copy link
Member

Choose a reason for hiding this comment

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

How is it that this is new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't precise enough

This is new:

Prisma formats the schema in node_modules automatically

They didn't do that and we could do our diff check without formatting our output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. One question: How come we aren't comparing the ASTs? Seems more robust.

I'm guessing it was probably too much work for too litle gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are in the business of comparing checksums like in all other places in the Wasp compiler :) that's why the source files needed to be the same.

@infomiho
Copy link
Contributor Author

infomiho commented Sep 2, 2024

I assume you did a "diff" between the Prisma version we were on and Prisma 5, by checking their Changelog

According to their migration guide to v5: min. version of Node.js, Typescript and PostgreSQL changes, rejectOnNotFound was removed (we didn't use it) and some other stuff: https://www.prisma.io/docs/orm/more/upgrade-guides/upgrading-versions/upgrading-to-prisma-5#removal-of-array-shortcuts%23removal-of-runtimeindexjs-from-generated-client

But nothing that major. I'll go over the release notes to see if I missed something 👍

is there anything new we should be adding support for in Wasp, or taht significantly changed?

There are new things, yes! I'm excited for their join support which would optimise some of our auth DB code - but I wouldn't really do it in this PR. I'll create an issue for that.

Seems from this PR that not much changed on the API level?

Yep, they kept things pretty much the same - which is great for us and our users :)

@Martinsos
Copy link
Member

is there anything new we should be adding support for in Wasp, or taht significantly changed?

Awesome! Here I was asking more in the direction of, did they add something that we need to put extra effort to enable people to use it, becaues Wasp somehow doesn't allow it? But it sounds like there is nothing like that, at least nothing major.

Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Looks great!

Please just augment that comment and merge when ready.

@@ -5,11 +5,8 @@ import { prisma } from 'wasp/server'
import { type {= userEntityUpper =} } from "wasp/entities"

const prismaAdapter = new PrismaAdapter(
// Using `as any` here since Lucia's model types are not compatible with Prisma 4
// model types. This is a temporary workaround until we migrate to Prisma 5.
// This **works** in runtime, but Typescript complains about it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Always love seeing these kind of changes!

@@ -91,7 +91,10 @@ export type GetQueryResolved = typeof _waspGetQuery

{=# crud.operations.Create =}
{=^ overrides.Create.isDefined =}
type CreateInput = Prisma.{= crud.entityUpper =}CreateInput
type CreateInput = Prisma.XOR<
Copy link
Contributor

Choose a reason for hiding this comment

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

I trust this is correct, but can you give more details?

Asking just out of curiosity.

>,
// TODO: (FILIP) Prisma errors casuing this test to fail, try to add Except
// after updating Prisma: https://github.com/wasp-lang/wasp/issues/2099
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Glad to see this is working now.

@@ -0,0 +1,14 @@
// Without this import, Prisma types are resolved incorrectly.
Copy link
Contributor

Choose a reason for hiding this comment

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

The code comment doesn't fully explain what's happening and what the tradeoffs are. Do you mind adding this playground link too.

It currently implies that type types have an implicit index signature (which isn't really true, they only exhibit special subtyping behavior). The comment also doesn't mention the compromise we made by declaring an extension to the FieldRef interface.

Anyway, both things should be clear from the playground, and you can adjust it as you see fit.

BTW, what happend with trying to fix the root of the problem, the definition of the JSONSerializable type (as attempted here). Did Superjson make it too complex?

-- | One of the checks we perform is to compare the Wasp generated schema.prisma file
-- and the schema.prisma file in the node_modules. Prisma formats the schema in node_modules
-- automatically, so we have to do the same to be able to compare them.
formatPrismaSchemaFileOnDisk :: Path' Abs (Dir ProjectRootDir) -> IO ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. One question: How come we aren't comparing the ASTs? Seems more robust.

I'm guessing it was probably too much work for too litle gain.

@infomiho infomiho merged commit ac38c6a into main Sep 4, 2024
6 checks passed
@infomiho infomiho deleted the miho-prisma-5 branch September 4, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix strange Prisma inferred type errors Fix Full-stack type safety type names Migrate to latest Prisma
3 participants