Skip to content

fix: pass only first language to next-server to match platform redirects support #2138

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

Merged
merged 6 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions cypress/e2e/default/i18n.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,34 @@ describe('Localization', () => {

cy.findByText('The current locale is en')
})

it(`function handler doesn't produce locale when first language in 'Accept-Language' header is not matching any of locales`, () => {
cy.request({
url: `/`,
followRedirect: false,
headers: {
// jp is not in i18n config, fr is in config
// Netlify Redirects only match on first language
// https://docs.netlify.com/routing/redirects/redirect-options/#redirect-by-country-or-language
// > Language-based redirects always match against the first language reported by the browser in the Accept-Language header regardless of quality value weighting.
// while Next.js matches on every language: https://github.com/vercel/next.js/blob/5d9597879c46b383d595d6f7b37fd373325b7544/test/unit/accept-headers.test.ts
'Accept-Language': 'jp, fr;q=0.9',
// make sure we don't use cached results
cookie: '__prerender_bypass=1',
// above cookie header cause us to hit non-ODB function variant
// below header allow us to force ODB-only code path despite not running ODB (this is just for testing purposes)
'x-next-just-first-accept-language': '1',
},
}).then((response) => {
// make sure we didn't hit SSR handler - not ODB and nothing else handles this request
// once platform starts supporting more languages in Accept-Language header - this test
// will start failing because then we will get platform level redirect
expect(response.headers['x-nf-render-mode']).to.eq('ssr')

// we don't expect for function handler to respond with a redirect
// locale redirects should be handled by Netlify redirects
// otherwise we could wrongly cache locale redirect generated by ODB
expect(response.status).to.eq(200)
})
})
})
10 changes: 10 additions & 0 deletions packages/runtime/src/templates/getHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,16 @@ const makeHandler = ({ conf, app, pageRoot, NextServer, staticManifest = [], mod
const query = new URLSearchParams(event.queryStringParameters).toString()
event.path = query ? `${event.path}?${query}` : event.path

if (event.headers['accept-language'] && (mode === 'odb' || event.headers['x-next-just-first-accept-language'])) {
// keep just first language to match Netlify redirect limitation:
// https://docs.netlify.com/routing/redirects/redirect-options/#redirect-by-country-or-language
// > Language-based redirects always match against the first language reported by the browser in the Accept-Language header regardless of quality value weighting.
// If we wouldn't keep just first language, it's possible for `next-server` to generate locale redirect that could be cached by ODB
// because it matches on every language listed: https://github.com/vercel/next.js/blob/5d9597879c46b383d595d6f7b37fd373325b7544/test/unit/accept-headers.test.ts
// 'x-next-just-first-accept-language' header is escape hatch to be able to hit this code for tests (both automated and manual)
event.headers['accept-language'] = event.headers['accept-language'].replace(/\s*,.*$/, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this should be done just for odb mode, and let ssr keep all languages, but it feels weird to change behavior between 2 of them?

Copy link
Contributor

@orinokai orinokai May 26, 2023

Choose a reason for hiding this comment

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

That's a good point, but worse, if we change it for SSR too it's going to be a breaking change because it will stop Next.js handling non-primary locales, meaning users with secondary/tertiary languages in their accept-language header will no longer see content in their language on SSR pages 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Having different behaviour between SSR/ODB feels wrong, and I guess it's because what we're doing is actually trying to workaround our lack of support by imposing the same limitation on Next, which would be a regression.

Copy link
Contributor Author

@pieh pieh May 26, 2023

Choose a reason for hiding this comment

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

Yeah, to me it's picking one of 2 bad options and figuring it which one is worse/better. I do agree it's probably better to keep SSR pages behaving as they are (so supporting matching on all languages on the header, even if there are multiple source that can produce redirect) and limit the workaround to just ODB (it does technically make things worse to match on lowest common denominator to avoid caching wrong things, so we want to limit area we make ~worse).

In any case 4c85c21 limits the workaround just to ODB

}

const { headers, ...result } = await getBridge(event, context).launcher(event, context)

// Convert all headers to multiValueHeaders
Expand Down