Updated the locale resolver to ECMA402's ResolveLocale #19460
Conversation
…cale This is an attempt to fix vercel#18676 Signed-off-by: Arthur Guiot <[email protected]>
arguiot
requested review from
ijjk,
lfades,
Timer and
timneutkens
as code owners
Stats from current PRDefault Server Mode (Increase detected
|
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| buildDuration | 10.1s | 10.2s | |
| nodeModulesSize | 84.9 MB | 85.4 MB |
Page Load Tests Overall increase ✓
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 2.266 | 2.214 | -0.05 |
| / avg req/sec | 1103.09 | 1128.97 | +25.88 |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.333 | 1.265 | -0.07 |
| /error-in-render avg req/sec | 1875.53 | 1975.88 | +100.35 |
Client Bundles (main, webpack, commons)
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| 677f882d2ed8..8b81.js gzip | 12.8 kB | 12.8 kB | ✓ |
| framework.HASH.js gzip | 39 kB | 39 kB | ✓ |
| main-0103832..fd40.js gzip | 6.54 kB | 6.54 kB | ✓ |
| webpack-e067..f178.js gzip | 751 B | 751 B | ✓ |
| Overall change | 59 kB | 59 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| polyfills-4b..e242.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| _app-3b0cf13..85f8.js gzip | 1.28 kB | 1.28 kB | ✓ |
| _error-6f635..c393.js gzip | 3.44 kB | 3.44 kB | ✓ |
| hooks-d4ffc3..9e0f.js gzip | 887 B | 887 B | ✓ |
| index-17468f..5d83.js gzip | 227 B | 227 B | ✓ |
| link-b618194..5477.js gzip | 1.61 kB | 1.61 kB | ✓ |
| routerDirect..924c.js gzip | 284 B | 284 B | ✓ |
| withRouter-7..c13d.js gzip | 284 B | 284 B | ✓ |
| Overall change | 8.01 kB | 8.01 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 321 B | 321 B | ✓ |
| Overall change | 321 B | 321 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| index.html gzip | 615 B | 615 B | ✓ |
| link.html gzip | 622 B | 622 B | ✓ |
| withRouter.html gzip | 609 B | 609 B | ✓ |
| Overall change | 1.85 kB | 1.85 kB | ✓ |
Serverless Mode
General Overall increase ⚠️
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| buildDuration | 11.7s | 11.9s | |
| nodeModulesSize | 84.9 MB | 85.4 MB |
Client Bundles (main, webpack, commons)
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| 677f882d2ed8..8b81.js gzip | 12.8 kB | 12.8 kB | ✓ |
| framework.HASH.js gzip | 39 kB | 39 kB | ✓ |
| main-0103832..fd40.js gzip | 6.54 kB | 6.54 kB | ✓ |
| webpack-e067..f178.js gzip | 751 B | 751 B | ✓ |
| Overall change | 59 kB | 59 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| polyfills-4b..e242.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| _app-3b0cf13..85f8.js gzip | 1.28 kB | 1.28 kB | ✓ |
| _error-6f635..c393.js gzip | 3.44 kB | 3.44 kB | ✓ |
| hooks-d4ffc3..9e0f.js gzip | 887 B | 887 B | ✓ |
| index-17468f..5d83.js gzip | 227 B | 227 B | ✓ |
| link-b618194..5477.js gzip | 1.61 kB | 1.61 kB | ✓ |
| routerDirect..924c.js gzip | 284 B | 284 B | ✓ |
| withRouter-7..c13d.js gzip | 284 B | 284 B | ✓ |
| Overall change | 8.01 kB | 8.01 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 321 B | 321 B | ✓ |
| Overall change | 321 B | 321 B | ✓ |
Serverless bundles
| vercel/next.js canary | arguiot/next.js locale-negocation | Change | |
|---|---|---|---|
| _error.js | 915 kB | 915 kB | ✓ |
| 404.html | 2.67 kB | 2.67 kB | ✓ |
| hooks.html | 1.92 kB | 1.92 kB | ✓ |
| index.js | 915 kB | 915 kB | ✓ |
| link.js | 973 kB | 973 kB | ✓ |
| routerDirect.js | 966 kB | 966 kB | ✓ |
| withRouter.js | 966 kB | 966 kB | ✓ |
| Overall change | 4.74 MB | 4.74 MB | ✓ |
|
The ResolveLocale usage LGTM but I defer to Vercel team on header parsing. |
| @@ -324,10 +324,40 @@ export default class Server { | |||
|
|
|||
| let defaultLocale = i18n.defaultLocale | |||
| let detectedLocale = detectLocaleCookie(req, i18n.locales) | |||
|
|
|||
| // Parse the `accept-language` header | |||
| const regex = /((([a-zA-Z]+(-[a-zA-Z0-9]+){0,2})|\*)(;q=[0-1](\.[0-9]+)?)?)*/g | |||
There was a problem hiding this comment.
More tests on this would be nice. Better yet if there's already something OSS that gives you the full list we should just use that.
The regex seems to be a little too restrictive compared to the header spec expectation.
There was a problem hiding this comment.
@timneutkens There are already several tests in i18n-support that test the detection from the accept-language header and they all seem to pass. How would you like me to test the regex? Is this something that should be included in the NextJS tests or would you want me to test it on my own (which is not ideal)?
There was a problem hiding this comment.
At 1st glance seems like regex doesn't fully match es-419-nu-hebr-x-private;q=1 for example
There was a problem hiding this comment.
I created the regex based on: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html. I may have made a mistake somewhere, but for the moment, I never found an example where the regex was handling the header incorrectly. I think the best way to make sure it works is by trying to find example where it doesn't work (and if we can't then it's fine).
There was a problem hiding this comment.
hmm you prob would have to implement the full RFC4647 (sans the lookup section). RFC2616 has been deprecated since 2014.
Are the no OSS lib for this?
There was a problem hiding this comment.
@longlho That may be me not understanding the specification correctly, but to me es-419-nu-hebr-x-private isn't a language-range? According to the specification language-range = ( ( 1*8ALPHA *( "-" 1*8ALPHA ) ) | "*" )
Is that correct or am I wrong?
There was a problem hiding this comment.
The language tag grammar in the rfc you linked has been deprecated since 2014.
There was a problem hiding this comment.
Ok, so that's why it got your example wrong. What is the updated specification ?
|
Not sure if I should keep this open as #19552 fix the same issue. I'll close this PR once the other is merged |
|
Seems good to close this in favor of #19552 so we can track this in one place, thanks for taking the time to open this PR! |
Fix for #18676 - The local negotiation was not done correctly with @hapi/accept, so I propose to use a more standard system (part of the ECMA402 specification, implemented by FormatJS) as suggested by @longlho .
The system is relatively simple because it relies on the
ResolveLocalefunction which allows to choose the locale correctly.