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

feat: upgrade prisma to 2.19.0 #130

Merged
merged 11 commits into from
Apr 2, 2021
Merged

Conversation

code-jenn-or
Copy link
Contributor

This PR upgrades Prisma to take advantage of the latest migrations and features available.

Changes

  • Resets the Migrations to the new SQL format
  • Updates the the schema to match the new nexus-prisma-plugin format
  • Updates the db context accessors to utilize the updates findUnique instead of findOne

Screenshots

(prefer animated gif)

Checklist

  • Requires dependency update?
  • Generating a new app works

Fixes #xxx

@code-jenn-or code-jenn-or requested a review from cball March 23, 2021 23:50
"db:migrate": "yarn -s prisma migrate up --experimental && yarn build:prisma",
"db:rollback": "yarn prisma migrate down --experimental && yarn build:prisma",
"db:migrate": "yarn -s prisma migrate deploy && yarn build:prisma",
"db:rollback": "yarn prisma migrate reset && yarn build:prisma",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is reset correct here?
I think of reset as a Full Reset, not a single rollback?

e.g.
db:reset
db:rollback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is great feedback! I think we should rename it. It does actually do a full reset and replays migrations. I need to dig back through but there wasn't a rollback per say, I think you literally need to reverse the SQL migration or do via a migration.

Copy link
Contributor

@mthomps4 mthomps4 Mar 26, 2021

Choose a reason for hiding this comment

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

@cmejet

Weird... that seems like an oversight on their part.

--

Actually just realized the new SQL migrations aren't in an up/down flow.
Maybe they add that in the next run... 🙏🏼

--

I would def rename this to reset if that is the case.
We can figure out what a true rollback looks like later. 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good article I found as well about this specifically. I also like that they made a change that when you call reset it will run the seed file if it detects it there.

https://www.prisma.io/docs/concepts/components/prisma-migrate/prisma-migrate-limitations-issues#lack-of-rollbacks--down-migrations

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about this more and chatting with Cory on Nest/CS work -- I get the reasoning... Still feels odd, but I get it.

Comment on lines +1 to +35
-- CreateEnum
CREATE TYPE "Role" AS ENUM ('ADMIN', 'USER');

-- CreateTable
CREATE TABLE "Profile" (
"id" TEXT NOT NULL,
"userId" TEXT NOT NULL,
"firstName" TEXT NOT NULL,
"lastName" TEXT NOT NULL,
"createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMP(3) NOT NULL,

PRIMARY KEY ("id")
);

-- CreateTable
CREATE TABLE "User" (
"id" TEXT NOT NULL,
"email" TEXT NOT NULL,
"password" TEXT NOT NULL,
"roles" "Role"[],
"createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMP(3) NOT NULL,

PRIMARY KEY ("id")
);

-- CreateIndex
CREATE UNIQUE INDEX "Profile.userId_unique" ON "Profile"("userId");

-- CreateIndex
CREATE UNIQUE INDEX "User.email_unique" ON "User"("email");

-- AddForeignKey
ALTER TABLE "Profile" ADD FOREIGN KEY ("userId") REFERENCES "User"("id") ON DELETE CASCADE ON UPDATE CASCADE;
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful

@@ -16,7 +16,7 @@ export async function createContext(context: ApolloApiContext): Promise<Context>
let user: User | null = null;

if (authHeader) {
user = await prisma.user.findOne({ where: { id: authHeader.userId } });
user = await prisma.user.findUnique({ where: { id: authHeader.userId } });
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 is a syntax change with the newest Prisma

Choose a reason for hiding this comment

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

💯 had to do this recently in the CITJS repo as well

@@ -3,7 +3,7 @@ to: graphql/modules/<%= name %>.ts
---
<% camelized = h.inflection.camelize(name) -%>
<% plural = h.inflection.pluralize(camelized) -%>
import { objectType, extendType } from '@nexus/schema';
import { objectType, extendType } from 'nexus';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Library @nexus/schema was deprecated and moved to nexus

@@ -178,6 +178,6 @@ export const SignupInput = inputObjectType({
definition: (t) => {
t.string('email', { required: true });
t.string('password', { required: true });
t.field('profile', { type: 'ProfileCreateOneWithoutUserInput' });
t.field('profile', { type: 'ProfileCreateNestedOneWithoutUserInput' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another syntax change with latest Prisma, discovered through build error and tracking down type generations

@@ -13,6 +13,7 @@ export const schema = makeSchema({
types,
plugins: [
fieldAuthorizePlugin(),
declarativeWrappingPlugin(),
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 was required to add to the configuration in order to satisfy some of the current schema definitions we had.
https://nexusjs.org/docs/plugins/declarativeWrapping

alias: 'db',
},
{
source: path.join(currentDirectory, 'graphql', 'context.ts'),
module: path.join(currentDirectory, 'graphql', 'context.ts'),
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 is the updated config Syntax, not documented well for the migration

@@ -6,14 +6,15 @@
"cacheDirectories": [".next/cache"],
<% } -%>
"scripts": {
"build": "ts-node ./scripts/buildProd",
"build": "node ./scripts/buildProd",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to a js file due to build errors that started occurring. The script did not appear to need to be typescript as it was already using exports.module syntax and an empty export.

Choose a reason for hiding this comment

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

I fixed this in the CITJS repo by overriding the module definition in the CLI params:

"ts-node": "ts-node --compiler-options '{\"module\":\"CommonJS\"}'",

Prisma has some notes on this:

https://www.prisma.io/docs/guides/application-lifecycle/seed-database#set-up-seeding-in-typescript

"db:rollback": "yarn prisma migrate down --experimental && yarn build:prisma",
"db:migrate": "yarn -s prisma migrate dev && yarn build:prisma",
"db:deploy": "yarn -s primsa deploy && yarn build:prisma",
"db:reset": "yarn prisma migrate reset && yarn build:prisma",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prisma no longer offers a direct rollback feature or migrate down so we renamed the script to match the functionality which resets the database and then runs the seed file if found. This is intended to be used on dev only, not on production database.

"db:migrate": "yarn -s prisma migrate up --experimental && yarn build:prisma",
"db:rollback": "yarn prisma migrate down --experimental && yarn build:prisma",
"db:migrate": "yarn -s prisma migrate dev && yarn build:prisma",
"db:deploy": "yarn -s primsa deploy && yarn build:prisma",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prisma no longer uses the migrate up command and instead favors deploy

@@ -22,7 +23,7 @@
"g:component": "hygen component new --name",
"g:graphql": "hygen graphql new --name",
"g:page": "hygen page new --name",
"g:migration": "yarn -s prisma migrate save --experimental",
"g:migration": "yarn -s prisma migrate dev",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated syntax for prisma 2.19.0

"start-server-and-test": "^1.11.2",
"supertest": "^4.0.2",
"ts-jest": "^26.1.3",
"ts-node-dev": "^1.0.0",
"typescript": "^3.9.7"
"typescript": "^4.1.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to match peer dependencies.

@@ -8,7 +8,7 @@
"forceConsistentCasingInFileNames": true,
"noEmit": true,
"esModuleInterop": true,
"module": "esnext",
"module": "commonjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All other tsconfig files are using commonjs and this started creating a build error when running buildProd, it was not happy with the empty export.

Choose a reason for hiding this comment

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

See my comment above, you can still set this to esnext and override at the command level

Copy link

@mcavaliere mcavaliere left a comment

Choose a reason for hiding this comment

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

Small comments - only blocking change I see is trying the commonjs override so we can keep using ts-node for the build script.

@@ -16,7 +16,7 @@ export async function createContext(context: ApolloApiContext): Promise<Context>
let user: User | null = null;

if (authHeader) {
user = await prisma.user.findOne({ where: { id: authHeader.userId } });
user = await prisma.user.findUnique({ where: { id: authHeader.userId } });

Choose a reason for hiding this comment

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

💯 had to do this recently in the CITJS repo as well

@@ -6,14 +6,15 @@
"cacheDirectories": [".next/cache"],
<% } -%>
"scripts": {
"build": "ts-node ./scripts/buildProd",
"build": "node ./scripts/buildProd",

Choose a reason for hiding this comment

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

I fixed this in the CITJS repo by overriding the module definition in the CLI params:

"ts-node": "ts-node --compiler-options '{\"module\":\"CommonJS\"}'",

Prisma has some notes on this:

https://www.prisma.io/docs/guides/application-lifecycle/seed-database#set-up-seeding-in-typescript

"@graphql-codegen/typescript-operations": "^1.17.6",
"@graphql-codegen/typescript-react-apollo": "1.17.6",
"@graphql-codegen/typescript-resolvers": "1.17.4",
"@prisma/cli": "2.8.1",

Choose a reason for hiding this comment

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

👏

@@ -8,7 +8,7 @@
"forceConsistentCasingInFileNames": true,
"noEmit": true,
"esModuleInterop": true,
"module": "esnext",
"module": "commonjs",

Choose a reason for hiding this comment

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

See my comment above, you can still set this to esnext and override at the command level

Copy link
Contributor

@MariahGrey MariahGrey left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@mcavaliere mcavaliere left a comment

Choose a reason for hiding this comment

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

LGTM

@code-jenn-or code-jenn-or merged commit ebc68d9 into canary Apr 2, 2021
@code-jenn-or code-jenn-or deleted the codejenn/upgrade-nexus-prisma branch April 2, 2021 14:48
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.

5 participants