-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
db: Seed on dev server startup #10599
Conversation
🦋 Changeset detectedLatest commit: eb0a7e9 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to see this! Left a query about integrations.
Will save my best puns for an approving seed
quel to this review.
logger.info( | ||
connectToStudio ? 'Connected to remote database.' : 'New local database created.' | ||
); | ||
const seedFileUrls = SEED_DEV_FILE_NAME.map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also need to account for integration seed files? Right now it looks like it’s only our static array of project ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's a good point! This is only for eagerly loading when a seed file changes, so it shouldn't break existing behavior. Happy to add if it's easy enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: this will work for relative paths but not package ids. Since eagerly reloading is a nice-to-have, I think this is a fine compromise.
To be clear, eagerly seeding on startup still works! This would only be the nuance of updating your integration in real time and expecting an eager reload. You'll need to visit a page in your browser to see that reload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I left a minor nitpick
.map((s) => (typeof s === 'string' && s.startsWith('.') ? new URL(s, root) : s)) | ||
.filter((s): s is URL => s instanceof URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you should flip them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see where you're coming from but we actually cannot flip these. The map()
is used to convert relative path strings to URLs, then the filter()
consolidates to all URL objects. If we put the filter()
first, those relative path strings would get filtered out before they were converted
Changes
This loads your seed file whenever the dev server starts. To make it unblocking and preserve HMR on file changes, we use
vite.ssrLoadModule()
.ssrLoadModule()
call for theastro:db:seed
module on server startupTesting
Manual testing to ensure logs fire and the seed file is only loaded/reloaded once.
Docs
N/A