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

On wasp db migrate-dev, if TS compilation failed, we don't do actual migration #2202

Open
Martinsos opened this issue Jul 23, 2024 · 2 comments
Assignees
Labels
prisma shouldfix We should do/fix this at some point

Comments

@Martinsos
Copy link
Member

Here is related discord convo, this caused quite some confusion: https://discord.com/channels/686873244791210014/1263910702117163119/1263910702117163119 .

Here are the steps to reproduce -- wasp v0.13.2 via Open SaaS

  • create a saas app with wasp new (select 3 for saas)
  • cd into app and start the db. Run the initial migration with wasp db migrate-dev
  • in main.wasp define a property on User entity such as myField String?
  • run wasp db migrate-dev
  • observe the TS failure stemming from /server/scripts/usersSeed.ts prevents you from being able to name/save the migration.

What happens is that if prisma schema is changed in such way that the new Prisma SDK is causing TS errors in the project (which is likely), we don't do actual migration.
Ok, so we probably first do prisma generate , then try to compile the Typescript project, and only then prisma migrate. I wonder if we should change this: we could run prisma migrate even if TS compilation fails. That sounds like it might be somewhat more correct / expected, and is in spirit with TS compilation philosophy. Seems that is what you also expected in this situation.

Even if not, we should make it clear in the error message what happened.

But I would suggest we revisit the wasp db migrate-dev and make sure we do perform actual migration even if TS compilation fails.

@Martinsos Martinsos added prisma shouldfix We should do/fix this at some point labels Jul 23, 2024
@sodic sodic self-assigned this Aug 2, 2024
@sodic
Copy link
Contributor

sodic commented Aug 23, 2024

The problem

Here's what happens with wasp db migrate-dev:

  1. The user changes the prisma.schema file.
  2. The user runs wasp db migrate-dev.
  3. Wasp generates the new Prisma client.
  4. Wasp builds the SDK, which also builds the user code that's copied into its ext-src folder.
  5. The SDK build fails because the old user code in ext-src doesn't fit the new Prisma client.
  6. Wasp exists before actually migrating the database.

Proposed solutions

@Martinsos identified two possible approaches:

  1. Tolerating the TypeScript errors and doing the migration anyway.
  2. Migrating the database before generating the client.

Tolerating TypeScript errors

I decided against this approach because:

  • Changing how we treat TypeScript errors during the SDK build would change it for every command, not just migrate-dev, which might have unintended consequences. I could have added a flag that tolerates TS errors only for migrate-dev, but it would have been too hacky IMO.
  • The SDK build only fails because it compiles user code. We should be compiling the user code before building the SDK (tracked here). Ignoring all SDK build errors just because they accidentally catch errors that should have been caught earlier seems incorrect and...
  • ... Most importantly, this issue is semi-blocked by #1718. Any fixes we do now might become incorrect or ineffective once we change the compilation order, meaning we'd have to fix the same thing again pretty soon.

Migrating before generating the client

This is the correct thing to do since it's natural to generate the database client after taking care of the database.
But it's not as easy as it sounds.

By default, Prisma generates the client after each migration. We turned this off in #962 because, at the time, we needed two different clients (source).

Since we no longer need two clients, my first thought was to turn the automatic client generation back on and stop generating the client manually. Unfortunately, we still want to generate the client during wasp compile, so that wasn't an option (see why: #444).

We could add a flag that tells wasp compile not to generate the client when dealing with wasp db migrate-dev, but this is a little hacky.

The proper solution (IMO)

It's best that each command only runs the compilation steps it needs.

Wasp goes through these steps before running each database command (as part of runDbCommand), but not all commands need all steps:

  1. Install the dependencies -- All commands need it because it installs the Prisma CLI
  2. Generate code into .wasp/out - All commands need it. .wasp/out/server is their working directory1
  3. Generate the Prisma client if necessary - Only seed needs this step because it's the only one that executes user code on the database.
  4. Build the SDK in .wasp/out/sdk/wasp - Only seed needs this step because it's the only one that executes user code on the database.
  5. Connect to the database - All commands need it.

The wasp db migrate-dev doesn't even need to run the SDK build that's causing the problem. Unfortunately, steps 1-4 are part of the compilation process (wasp compile) and can't be cherry-picked.

Therefore, to make this solution possible, we'd have to modularize the compile function, which is a significant change.

[1] Technically, `seed` is the only command that doesn't need `.wasp/out/db`, but I think there's no need to go that deep. It's cleaner if we just pretend it does.

@Martinsos
Copy link
Member Author

Thanks for the detailed analysis @sodic !
So you are saying, let's not ignore TS errors in that case, let's instead not do TS compilation (of user's and SDK code) at all, because it is not even needed for the db migrate-dev.
Ok, makes sense! But to not do it, we should modularize compile function in waspc, so we can indeed skip SDK build / whatever we need to skip.

That makes sense to me, but quick idea: what if we ran compile as normal, let errors be printed and all, but then just continue with migration even if compile failed? Hm but I guess we won't be sure what failed, maybe it was TS errors maybe it was something else. Althogh we could maybe modify it to get that level of info.

Ok, we will be able to figure out the details when we get into it, but it makes sense to me general. I wonder if we are loosing anything by not building user code / Wasp SDK code when doing migration, are we twarthing some expectations from user, but I guess not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prisma shouldfix We should do/fix this at some point
Projects
None yet
Development

No branches or pull requests

2 participants