Skip to content

Commit

Permalink
Fix metadata url cases should not append with trailing slash (#63050)
Browse files Browse the repository at this point in the history
### What

Exclude the cases like external urls and relative urls with query from
appending trailing slash when it's needed.

The process is:
- If it's a uncertain string path (relative url, could start with `'./'`
or `/`), convert to relative that starts with `/`;
- then we covert the url (string or URL) to string url
- We do the check if we need to append the trailing slash

### Why

In #62109 we added functionality that can only append trailing slash
when we appended trailing slash to some metadata url like `canonical`
url and open graph url when the config is enabled.
For urls with queries, the trailing slash can also be omitted.
For the external urls (different origin comparing to `metadataBase`) we
don't need to append trailing slash as they're not the same web app.

x-ref: [slack
thread](https://vercel.slack.com/archives/C0676QZBWKS/p1709845946033929)

Closes NEXT-2762
Closes NEXT-2753
  • Loading branch information
huozhi committed Mar 18, 2024
1 parent a3707f5 commit 531cdb5
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 9 deletions.
66 changes: 65 additions & 1 deletion packages/next/src/lib/metadata/resolvers/resolve-url.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { resolveUrl } from './resolve-url'
import { resolveUrl, resolveAbsoluteUrlWithPathname } from './resolve-url'

// required to be resolved as URL with resolveUrl()
describe('metadata: resolveUrl', () => {
Expand Down Expand Up @@ -42,3 +42,67 @@ describe('metadata: resolveUrl', () => {
)
})
})

describe('resolveAbsoluteUrlWithPathname', () => {
describe('trailingSlash is false', () => {
const metadataBase = new URL('https://example.com/')
const opts = {
trailingSlash: false,
pathname: '/',
}
const resolver = (url: string | URL) =>
resolveAbsoluteUrlWithPathname(url, metadataBase, opts)
it('should resolve absolute internal url', () => {
expect(resolver('https://example.com/foo')).toBe(
'https://example.com/foo'
)
})
})

describe('trailingSlash is true', () => {
const metadataBase = new URL('https://example.com/')
const opts = {
trailingSlash: true,
pathname: '/',
}
const resolver = (url: string | URL) =>
resolveAbsoluteUrlWithPathname(url, metadataBase, opts)
it('should add trailing slash to relative url', () => {
expect(resolver('/foo')).toBe('https://example.com/foo/')
})

it('should add trailing slash to absolute internal url', () => {
expect(resolver('https://example.com/foo')).toBe(
'https://example.com/foo/'
)
expect(resolver(new URL('https://example.com/foo'))).toBe(
'https://example.com/foo/'
)
})

it('should not add trailing slash to external url', () => {
expect(resolver('https://external.org/foo')).toBe(
'https://external.org/foo'
)
expect(resolver(new URL('https://external.org/foo'))).toBe(
'https://external.org/foo'
)
})

it('should not add trailing slash to absolute internal url with query', () => {
expect(resolver('https://example.com/foo?bar')).toBe(
'https://example.com/foo?bar'
)
expect(resolver(new URL('https://example.com/foo?bar'))).toBe(
'https://example.com/foo?bar'
)
})

it('should not add trailing slash to relative url with query', () => {
expect(resolver('/foo?bar')).toBe('https://example.com/foo?bar')
expect(resolver(new URL('/foo?bar', metadataBase))).toBe(
'https://example.com/foo?bar'
)
})
})
})
35 changes: 27 additions & 8 deletions packages/next/src/lib/metadata/resolvers/resolve-url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,40 @@ function resolveAbsoluteUrlWithPathname(
metadataBase: URL | null,
{ trailingSlash, pathname }: MetadataContext
): string {
// Resolve url with pathname that always starts with `/`
url = resolveRelativeUrl(url, pathname)

// Get canonicalUrl without trailing slash
let canonicalUrl = ''
// Convert string url or URL instance to absolute url string,
// if there's case needs to be resolved with metadataBase
let resolvedUrl = ''
const result = metadataBase ? resolveUrl(url, metadataBase) : url
if (typeof result === 'string') {
canonicalUrl = result
resolvedUrl = result
} else {
canonicalUrl = result.pathname === '/' ? result.origin : result.href
resolvedUrl = result.pathname === '/' ? result.origin : result.href
}

// Add trailing slash if it's enabled
return trailingSlash && !canonicalUrl.endsWith('/')
? `${canonicalUrl}/`
: canonicalUrl
// Add trailing slash if it's enabled for urls matches the condition
// - Not external, same origin with metadataBase
// - Doesn't have query
if (trailingSlash && !resolvedUrl.endsWith('/')) {
let isRelative = resolvedUrl.startsWith('/')
let isExternal = false
let hasQuery = resolvedUrl.includes('?')
if (!isRelative) {
try {
const parsedUrl = new URL(resolvedUrl)
isExternal =
metadataBase != null && parsedUrl.origin !== metadataBase.origin
} catch {
// If it's not a valid URL, treat it as external
isExternal = true
}
if (!isExternal && !hasQuery) return `${resolvedUrl}/`
}
}

return resolvedUrl
}

export {
Expand Down
14 changes: 14 additions & 0 deletions test/e2e/app-dir/trailingslash/app/metadata/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export default function Page() {
return 'page'
}

export const metadata = {
openGraph: {
// external url with different domain
url: 'http://trailingslash-another.com/metadata',
},
alternates: {
// relative url with query string
canonical: '/metadata?query=string',
},
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/trailingslash/trailingslash.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,15 @@ createNextDescribe(
.waitForElementByCss('#a-page')
expect(await browser.waitForElementByCss('#a-page').text()).toBe('A page')
})

it('should not add trailing slash to external url or relative url with query', async () => {
const $ = await next.render$('/metadata')
expect($('[rel="canonical"]').attr('href')).toBe(
'http://trailingslash.com/metadata?query=string'
)
expect($('[property="og:url"]').attr('content')).toBe(
'http://trailingslash-another.com/metadata'
)
})
}
)

0 comments on commit 531cdb5

Please sign in to comment.