Skip to content

Updates seeds script to be generic, no more errors throw on first run #10739

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

Merged
merged 20 commits into from
Jun 7, 2024

Conversation

cannikin
Copy link
Member

@cannikin cannikin commented Jun 5, 2024

If you edit the schema.prisma file and change the table name before running yarn rw prisma migrate dev the first time, you'll get a scary error at the end of your run:

Image

The error is caught, and doesn't stop the migration from running, but looks nasty. Everyone will see this when running through the tutorial as the first thing we do is change the schema.prisma file and then create a migration.

The seed should not contain any actual seed data, just comments about how to use it when the time comes. I've updated the template to be much simpler to start, and includes a link to a new doc I created that goes into more detail about how to use seeds, best practices, etc. The first time you migrate the console message will point you to the file, and the file will point you to the docs.


This PR (10739) also needs this to be complete: #10751

@cannikin cannikin requested a review from Josh-Walker-GM June 5, 2024 23:30
@cannikin cannikin added this to the next-release-patch milestone Jun 5, 2024
@cannikin cannikin added the release:fix This PR is a fix label Jun 5, 2024
@cannikin cannikin self-assigned this Jun 5, 2024
Copy link
Contributor

@Josh-Walker-GM Josh-Walker-GM left a comment

Choose a reason for hiding this comment

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

I like this. One comment on the docs and just the linting errors to silence but otherwise looks great to me.

// eslint-disable-next-line @typescript-eslint/no-unused-vars
for (const item of data) {
// Create a database record for each item in `data`
// ie: `await db.user.create({ data: item })`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take the opportunity to use createMany here now that SQLite supports it with Prisma? prisma/prisma#10710

Copy link
Member Author

Choose a reason for hiding this comment

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

So in the docs I talk about the seeds being idempotent, and running the file multiple times won't create new records. Unfortunately with createMany() it'll do just that. :( In the doc I talk about the tradeoffs and a fallback so that you can use createMany() but that you'd want to check first to make sure you're not creating duplicate records.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although that makes me realize even THIS code example should use upsert() instead of create() so follow that recommendation!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes. I would still check.

I just meant not looping and await each create.

if ((await db.user.count()) === 0) {
      await db.user.createMany({ data: users })
    } else {
      console.log('Users already seeded')
    }
      await db.post.createMany({ data: posts })
    } else {
      console.log('Posts already seeded')
    }```

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd leave a check for multiple seed runs and the createMany for initial db seed and db migration resets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, I was going even more granular than that and checking that each individual record was present in the database before creating/updating. All the time when I'm in dev and things are getting updated and deleted all over the place, I can always re-run the seed and be back to a known state with those fixed seeds: I don't want to completely recreate the database from scratch and start over, just get me back to a known minimum amount of data.

I'd leave a check for multiple seed runs and the createMany for initial db seed and db migration resets

How would you know from within the seed script that it's the initial/reset run and not a manual/multiple run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay you convinced me 😃 How's this?

// Create your database records here! For example, seed some users:
//
// const users = [
//   { name: 'Alice', email: '[email protected] },
//   { name: 'Bob', email: '[email protected] },
// ]
//
// await db.user.createMany({ data: users })

Comment on lines +81 to +83
the performance benefits of `createMany()`. Since you are running seeds
realitively rarely, it's our recommendation that you focus less on absolute
performance and worry more about making them easy to maintain.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this entire #### Performance section could just have been this 😀 You run seeds like once. Who cares about perf 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

haha I agree, although DT does make a good point with createMany(): in my later docs example with the book categories, there are apparently 3,000+ official ones! If you're doing a lot of CI that's one that could probably stand to be as fast as possible, and really no reason to do an upsert() since you won't be manually editing the list and need to restore changed records!

Copy link
Contributor

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

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

LGTM!

@cannikin cannikin merged commit 3ddbb4b into main Jun 7, 2024
50 checks passed
@cannikin cannikin deleted the rc-seeds-update branch June 7, 2024 17:51
@cannikin cannikin restored the rc-seeds-update branch June 7, 2024 17:55
Josh-Walker-GM added a commit that referenced this pull request Jun 9, 2024
…#10739)

If you edit the `schema.prisma` file and change the table name before
running `yarn rw prisma migrate dev` the first time, you'll get a scary
error at the end of your run:

![Image](https://github.com/orgs/redwoodjs/projects/18/assets/300/e0af73f0-3fa9-4b49-bfc9-b54fe9d8a868)

The error is caught, and doesn't stop the migration from running, but
looks nasty. Everyone will see this when running through the tutorial as
the first thing we do is change the `schema.prisma` file and then create
a migration.

The seed should not contain any actual seed data, just comments about
how to use it when the time comes. I've updated the template to be much
simpler to start, and includes a link to a new doc I created that goes
into more detail about how to use seeds, best practices, etc. The first
time you migrate the console message will point you to the file, and the
file will point you to the docs.

---------

Co-authored-by: Josh GM Walker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants