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.

Sign up for GitHub

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

feat(vercel): allow external redirects #11422

Closed
wants to merge 3 commits into from
Closed

feat(vercel): allow external redirects #11422

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5fb8a6
feat(vercel): allow external redirects
wotschofsky Jul 5, 2024
80ad25b
Use URL.canParse for detecting external redirects
wotschofsky Jul 21, 2024
1463b0e
Merge branch 'withastro:main' into main
wotschofsky Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clean-cows-draw.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/vercel': minor
---

Allow external redirects to be configured through Astro config
24 changes: 19 additions & 5 deletions packages/integrations/vercel/src/lib/redirects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,29 @@ function getReplacePattern(segments: RoutePart[][]) {
}

function getRedirectLocation(route: RouteData, config: AstroConfig): string {
let redirectPath: string;
let forceTrailingSlash = false;

if (route.redirectRoute) {
const pattern = getReplacePattern(route.redirectRoute.segments);
const path = config.trailingSlash === 'always' ? appendForwardSlash(pattern) : pattern;
return pathJoin(config.base, path);
redirectPath = getReplacePattern(route.redirectRoute.segments);
if (config.trailingSlash === 'always') forceTrailingSlash = true;
} else if (typeof route.redirect === 'object') {
return pathJoin(config.base, route.redirect.destination);
redirectPath = route.redirect.destination;
} else {
return pathJoin(config.base, route.redirect || '');
redirectPath = route.redirect || '';
}
Comment on lines +88 to +98
Copy link
Member

Choose a reason for hiding this comment

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

This code seems correct, but it might be more readable if we didn't use mutable state. What about splitting this:

function getRedirectDestination(route: RouteData): string {
	if (route.redirectRoute) {
		return getReplacePattern(route.redirectRoute.segments);
	}
	if (typeof route.redirect === 'object') {
		return route.redirect.destination;
	}
	return route.redirect || '';
}

function getRedirectLocation(route: RouteData, config: AstroConfig): string {
	const redirectDestination = getRedirectDestination(route);
	const forceTrailingSlash = config.trailingSlash === 'always';

Copy link
Author

Choose a reason for hiding this comment

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

Currently the trailingSlash rule is only considered in the first case. However I don't see a reason not to always apply it. If so forceTrailingSlash can be eliminated completely.

This version has significantly less mutable state. The only option I see for avoiding that completely, that doesn't introduce another level of complexity through a helper function, would be using ternary expression for conditionally appending the slash.

function getDestinationFromRoute(route: RouteData) {
	if (route.redirectRoute) {
		return getReplacePattern(route.redirectRoute.segments);
	}
	if (typeof route.redirect === 'object') {
		return route.redirect.destination;
	}
	return route.redirect || '';
}

function getRedirectLocation(route: RouteData, config: AstroConfig): string {
	let destination = getDestinationFromRoute(route);

	// Is a full URL - do not transform
	if (URL.canParse(destination)) {
		return destination;
	}

	if (config.trailingSlash === 'always') {
		destination = appendForwardSlash(destination);
	}

	return pathJoin(config.base, destination);
}


// Is external link - do not transform
const hasProtocol = redirectPath.includes('://');
if (hasProtocol) {
return redirectPath;
wotschofsky marked this conversation as resolved.
Show resolved Hide resolved
}

if (forceTrailingSlash) {
redirectPath = appendForwardSlash(redirectPath);
}

return pathJoin(config.base, redirectPath);
}

function getRedirectStatus(route: RouteData): number {
Expand Down
9 changes: 9 additions & 0 deletions packages/integrations/vercel/test/redirects.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ describe('Redirects', () => {
},
'/blog/[...slug]': '/team/articles/[...slug]',
'/Basic/http-2-0.html': '/posts/http2',
'/google': 'https://google.com'
},
trailingSlash: 'always',
});
Expand Down Expand Up @@ -56,6 +57,14 @@ describe('Redirects', () => {
assert.equal(staticRoute.status, 301);
});

it('define external redirects', async () => {
const config = await getConfig();

const route = config.routes.find((r) => r.src === '/google');
assert.equal(route.headers.Location, 'https://google.com');
assert.equal(route.status, 301);
});

it('defines dynamic routes', async () => {
const config = await getConfig();

Expand Down
Loading