Handled redirects to CDN in S3Storage #25667
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20052779429 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
| return function (_req: unknown, _res: unknown, next: (err?: unknown) => void) { | ||
| next(); | ||
| return ( | ||
| req: {path: string}, |
There was a problem hiding this comment.
let's import express types here and use them
allouis
force-pushed
the
push-tpmzsqqtpown
branch
from
e7a18be to
f5a62d4
Compare
ref https://linear.app/ghost/issue/PRO-1550 After moving to CDN based storage with the S3Storage adapter, there may still be references to the old URLs in the wild. Rather than 404 we want to redirect to the CDN on the assumption that files have been migrated.
allouis
force-pushed
the
push-tpmzsqqtpown
branch
from
f5a62d4 to
bc2ed0d
Compare
| } | ||
|
|
||
| const key = this.buildKey(relativePath); | ||
| res.redirect(301, `${this.cdnUrl}/${key}`); |
There was a problem hiding this comment.
Missing return statement on redirect? Not a bug here but adding return res.redirect(...) is a common pattern. Also makes it consistent with the early return return next();
ref https://linear.app/ghost/issue/PRO-1550
After moving to CDN based storage with the S3Storage adapter, there may still be references to the old URLs in the wild. Rather than 404 we want to redirect to the CDN on the assumption that files have been migrated.