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 seed to use export default function() instead of top-level await #10334

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Mar 5, 2024

Changes

  • Changes db seed file format to require exporting a default function instead of running the seed file at the top level:

    Before

    // db/seed.ts
    import { db, Table } from 'astro:db';
    
    await db.insert(Table).values({ foo: 'bar' });

    After

    // db/seed.ts
    import { db, Table } from 'astro:db';
    
    export default async function() {
    	await db.insert(Table).values({ foo: 'bar' });
    }
  • This makes re-running a seed file much less complex — no need to invalidate a module to get its top-level expressions to re-run.

  • Also avoids risks of bundlers one day having issues with top-level await.

  • This PR updates the seed files in test fixtures to the new format.

Testing

  • Existing tests should pass.
  • I also ran the fixtures with astro dev locally to check behaviour is as expected.

Docs

Will need to update the WIP docs to show this new format, cc @sarah11918

@delucis delucis requested a review from bholmesdev March 5, 2024 20:59
Copy link

changeset-bot bot commented Mar 5, 2024

🦋 Changeset detected

Latest commit: 7f0741c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matthewp
Copy link
Contributor

matthewp commented Mar 5, 2024

I'd like to discuss the problems this solves before we accept this change, as this feels like (albeit slight) DX downgrade for something that feels like an implementation detail / bug.

Can you explain the problems you've seen from the existing seeding method? Have you run into bugs or are these theoretical problems?

@bholmesdev
Copy link
Contributor

bholmesdev commented Mar 5, 2024

Yes, happy to reiterate @matthewp!

Problem: both users and integrations are able to seed the database. Trouble is how we handle changes to the user's seed file in development. When the user's seed file changes, we tear down the whole database, recreate, and rerun the user seed file. Trouble is, we also need to rerun the integration seed file to get the db back to its initial state. Since we use top-level await, and imports are cached, we cannot easily rerun the integration seed file.

Speaks to a general problem: top level await = can only run module once (easily).

One fix would be to stop tearing down the whole database, instead just tearing down user-managed tables to leave integration tables in-tact. The other fix is to add a wrapper function to ensure seed files can be rerun in development. We chose the latter because we're worried other problems could be caused in the future where rerunning the seed file is the only fix. I agree with you on ergonomics though. Curious which side you lean towards.

@delucis
Copy link
Member Author

delucis commented Mar 5, 2024

Can you explain the problems you've seen from the existing seeding method? Have you run into bugs or are these theoretical problems?

Real problems when introducing integration support. Here’s the rundown:

  • When the database is first used, seedLocal() function runs to populate it based on a user’s seed file. 👍

  • If the user edits their seed file, seedLocal() re-runs, wiping clean the database and populating it with the expressions in the seed file. 👍

So far so good. This gets wonky once you need to support multiple seed files (i.e. integrations):

  • When the database is first used, seedLocal() function runs to populate it based on a user’s seed file and zero to many integration seed files. 👍

  • If the user edits their seed file, seedLocal() re-runs, wiping clean the database. The user-edited seed file re-runs, because it changed, and Vite reloads it completely. However, importing the unchanged integration seed files does not re-run their top-level expressions, because those run once when the module is first added to the module graph. That means after a seed edit users only get a partially seeded database unless they restart the dev server. 😬

I shared a few options with Ben yesterday:

  1. Switch seed files to export a default seed function — easier for us to re-run, a bit of extra boilerplate for users.
  2. Same, but for integrations only — still easy for us, boilerplate only for integration authors, but then there’s a difference between integration and user seed files.
  3. Find some tool for force invalidating and rerunning an ES module — more complex for us, keeps current seed file format.
  4. Track who is doing what to which tables somehow and avoid blowing away all tables when a seed file changes — seems pretty tricky to get right, especially if you ended up with two seed files writing to the same table.

I’ll also note that with option 2, it could still lead to bad DX: while the obvious use case for an integration is to take care of seeding itself, you might also use an integration to separate out multiple seed files that are user-editable. In that case, the broken flow described above could also happen where the integration seed reloads successfully and the user seed doesn’t re-run.

@matthewp
Copy link
Contributor

matthewp commented Mar 6, 2024

👿 FINE 😀

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

defaultinitely ready to merge

@bholmesdev bholmesdev merged commit bad9b58 into main Mar 7, 2024
13 checks passed
@bholmesdev bholmesdev deleted the plt-1763/seed-format branch March 7, 2024 18:04
@astrobot-houston astrobot-houston mentioned this pull request Mar 7, 2024
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.

3 participants