Add scope to redirects #315
Conversation
auto-assign
bot
requested review from
dkarnutsch,
fraxachun,
nsams and
thomasdax98
|
|
||
| export class Migration20240729171532 extends Migration { | ||
| async up(): Promise<void> { | ||
| this.addSql('alter table "Redirect" add column "scope_domain" text not null;'); |
There was a problem hiding this comment.
You'd need to drop all redirects before adding this non-nullable column. But wouldn't it be better to add it to the one migration we already have?
There was a problem hiding this comment.
Why? It doesn't remove sth but just adds a column. Sure but that wouldn't make a difference for the first point right?
There was a problem hiding this comment.
Sure but that wouldn't make a difference for the first point right?
You're right.
But wouldn't it be better to add it to the one migration we already have?
How ever, I'd still at this line to the initial migration, which should be fine for all new projects.
| import { MikroOrmModule } from "@mikro-orm/nestjs"; | ||
| import { Module } from "@nestjs/common"; | ||
|
|
||
| import { Page } from "./entities/page.entity"; | ||
| import { PagesResolver } from "./pages.resolver"; | ||
|
|
||
| @Module({ | ||
| imports: [MikroOrmModule.forFeature([Page])], | ||
| imports: [MikroOrmModule.forFeature([Page]), RedirectsModule], |
There was a problem hiding this comment.
I don't think this import is needed
There was a problem hiding this comment.
How can I test if it is needed?
There was a problem hiding this comment.
Remove it and see if the API still starts
site/src/middleware.ts
Outdated
| @@ -1,25 +1,29 @@ | |||
| import { domain } from "@src/config"; | |||
There was a problem hiding this comment.
Please merge main and get the domain from the site config now that #283 has been merged
| export const createRedirects = async () => { | ||
| const key = `redirects`; | ||
| export const createRedirects = async (scope: GQLRedirectScope) => { | ||
| const key = `redirects-${JSON.stringify(scope)}`; |
There was a problem hiding this comment.
| const key = `redirects-${JSON.stringify(scope)}`; | |
| const key = `redirects-${scope.domain}`; |
site/src/middleware.ts
Outdated
| @@ -41,6 +42,7 @@ export async function middleware(request: NextRequest) { | |||
| } | |||
|
|
|||
| const siteConfig = await getSiteConfigForHost(host); | |||
| const scope = siteConfig?.scope.domain ? { domain: siteConfig?.scope.domain } : undefined; | |||
There was a problem hiding this comment.
You could get the scope after the if below, then you shouldn't need the undefined handling I suppose.
| @@ -19,5 +19,7 @@ export class Migration20220721122758 extends Migration { | |||
| 'alter table "PageTreeNode" add constraint "PageTreeNode_parentId_foreign" foreign key ("parentId") references "PageTreeNode" ("id") on update cascade on delete set null;', | |||
| ); | |||
| this.addSql('create index "PageTreeNode_parentId_index" on "PageTreeNode" ("parentId");'); | |||
|
|
|||
| this.addSql('alter table "Redirect" add column "scope_domain" text not null;'); | |||
There was a problem hiding this comment.
I don't think it is good practice to alter existing migrations. If the starter would be deployed, this change would not be made.
Create a new migration and also make it fail-safe by truncating the table before.
There was a problem hiding this comment.
#315 (comment)
Please discuss this with @johnnyomair and let me know what I should do ^^
There was a problem hiding this comment.
I'm sorry, this was my suggestion 😅 We did this multiple times in the past to avoid having a bunch of migrations when starting a new project. We could change our approach though
There was a problem hiding this comment.
@dkarnutsch what do you prefer?
There was a problem hiding this comment.
@nsams what do you think? Maybe we should start adding actually working migrations once the Starter has been deployed
There was a problem hiding this comment.
A growing Number of Migrations in New Projects Is Not desirable, that is unnecessarily confusing.
Maybe we can drop database and run fixtures on every starter deploy?
PR Checklist