Support for i18n in Next 10 #75
Conversation
|
Thanks @lindsaylevine! This does indeed seem to resolve the main issue. I'm able to deploy now. 🥳 However, it seems like default locales are only getting applied to dynamic routes.
This works locally with Also, as you mentioned, index routes cause build errors. Thanks for opening an issue. |
|
Hey @lindsaylevine seems that the code works! @justintemps probably this could be fixed with redirects right? |
|
hey yall - udpate on this while diagnosing the index route stuff. the i18n internals feel very unreliable to me right now. can read more here -> vercel/next.js#19126. that said, i'm not sure how quickly i can get this merged until i hear from them/feel a bit more confident about the index route and dataRoute ambiguities. |
|
@justintemps i just saw this edited part of your comment -- However, it seems like default locales are only getting applied to dynamic routes. /my/[dynamicRoute] gets the default language but /homepage gets the Netlify 404 page. I have to prepend the locale to make it work: /en-gb/homepage. This works locally with next start so I don't think the issue is on the next.js side. -- can you elaborate on that for me? or (sorry to ask) share a repro repo? are you saying |
lindsaylevine
force-pushed
the
ll/i18n
branch
from
a29c8e6
to
2b7c6e0
Compare
|
@lindsaylevine here's a repro repo: https://github.com/justintemps/next-js-blog I've deployed this to Netlify so you can see what I mean: https://priceless-wiles-007723.netlify.app
Hope that's useful. |
|
thank you so so much @justintemps, that will be enormously helpful! i'm hoping to pivot back to this soon, we've been busy over at github.com/netlify/netlify-plugin-nextjs which will be the next "iteration" of NoN 👀 . |
|
@justintemps - sorry for the delay fixing your issue. the recent commit should solve it. i forked your repo (thanks again for that!!!) and demo the fix here, which uses this branch's latest: https://justin-i18n-repro.netlify.app/. let me know if this works as intended! <3 |
|
This is awesome! Could y'all publish this to a canary/prerelease version on npm? No worries if not, but the fix would be majorly helpful to me. 😇 |
|
Thanks so much for working on this @lindsaylevine! I just have one issue which may boil down to my misunderstanding how i18n works with dynamic routes. These all work: These also work This works But this doesn't Which is curious because these do work (from my original repro repo) Feel like I'm missing something, but I'm not sure what. |
@TejasQ is there a benefit to you to having a canary version vs just installing the branch like |
|
That works too! |
|
Okay, so I've tried it and it doesn't work yet, but I've got all the confidence in y'all. |
|
@TejasQ ugh, yeah, Soon TM <3 can you tell me what page type is your index route / |
|
It's using More details:
|
|
@lindsaylevine's draft solves the problem here |
|
@justintemps just force-pushed a fixup. all the links in your last comment should work now!!! note for everyone else reading this reply: this is just a fix for the page types of justin's project/repo he provided (aka pages without props, see commit for more info). i'm still picking through the other page types. hoping to be done by next week! |
lindsaylevine
force-pushed
the
ll/i18n
branch
from
e1cda1a
to
a8bf845
Compare
|
@lindsaylevine yup, that does it. Thanks for all the work on this! |
|
hey all - was a little optimistic with my schedule this week and some pain points with redirects in this work. updated time estimation/goal would be a merge of this PR before the holidays. |
lindsaylevine
force-pushed
the
ll/i18n
branch
from
9ba1646
to
1b4cb5a
Compare
lindsaylevine
force-pushed
the
ll/i18n
branch
2 times, most recently
from
6b6147d
to
e010440
Compare
lindsaylevine
force-pushed
the
ll/i18n
branch
from
a691fef
to
60e8a49
Compare
There was a problem hiding this comment.
Wow, this is a BEAST!! 🤯🤯🤯 Fantastic job figuring out all these edge cases and managing all these irregularities with the prerender-manifest, etc... 🎉
I think i18n is still missing for two page types: getInitialProps and getStaticProps with fallback: true, but I think you'll be able to add those relatively easily. Maybe you can add two simple Cypress tests for them first (e.g., visit("/en/shows/:id")), so that we catch them in the future.
Then, there is one tiny bug that currently treats all pages with revalidate as if they were not revalidate pages. You already spotted this in the snapshot, where the revalidate page suddenly has an entry for preview mode, which of course it doesn't need because it's always SSRed. I flagged what I believe to be the issue's source in getPrerenderManifest. It's just one line.
Other than that, everything seems to work great. The other comments I made are just about style and naming and code location, which are really just food for thought. I'll let you call the shots on those! 💥
Let me know if something I wrote doesn't make sense.
Amazing work, @lindsaylevine 😊
| @@ -15,8 +9,7 @@ const getLocaleRoutesForI18n = ({ route, srcRoute }) => { | |||
| return i18n.locales.map((locale) => { | |||
| return { | |||
| route: `/${locale}${route}`, | |||
| dataRoute: getI18nDataRoute(route, locale), | |||
| nakedRoute: route, | |||
| dataRoute: getDataRouteForRoute(route, locale), | |||
| }; | |||
| }); | |||
| }; | |||
There was a problem hiding this comment.
rn this is only used by gsp/pages.js, might be able to do without this or rename it
There was a problem hiding this comment.
nvm its not used anywhere LuL
|
ubuntu 10 tests fail due to reported bug in 10.0.4 vercel/next.js#20456, which i upgraded to so we could remove the backslash pages-manifest patch options:
as number 2 isnt ideal, i will delay the fix and remove my recent commit |
- Support for i18n in Next 10 ([#75](#75)) - dependabot: node-notifier from 8.0.0 to 8.0.1 ([#125](#125)) - Expose Netlify function params as netlifyFunctionParams ([#119](#119)) - Fix: local cypress cache control ([#118](#118)) - Fix: add res.finished to createResponseObject ([#117](#117)) - Fix: Windows support ([#101](#101)) - Improve logs for specified functions/publish dirs ([#100](#100))
This PR makes the necessary changes to all page types to support i18n in Next 10 on Netlify
TO-DO:
and probably more