search

aralroca / next-translate-plugin

Next-translate plugin for i18n in Next.js 🌍 - Load page translations and use them in an easy way!
MIT License
32 stars 20 forks source link

Support App Router & Pages Router #22

Closed alvesvin closed 1 year ago

alvesvin commented 1 year ago

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update [x] Bug fix [ ] New feature [ ] Other, please explain:

What changes did you make? (Give an overview) The webpack loader selects the correct helpers on a page by page basis rather than experimental.appDir config.

Which issue (if any) does this pull request address?

21 #7 #23

Is there anything you'd like reviewers to focus on? Perhaps readability and size

alvesvin commented 1 year ago

Just a notice, this is incomplete yet. I'm going to be working on it later today.

alvesvin commented 1 year ago

Some updates:

Sub-path routing

Providing more context, in Next 12, when defining i18n options in next.config.js it is possible to navigate to the following routes:

/about loads the default language /en/about loads the en version of the page /br/about loads the br version of the page

For Next 13, users would need to create /app/[lang]/about/page.tsx (same for any other internationalized page). Together with that, users need to create a middleware.ts to redirect to the correct language sub-path. Then we would use page's props.params.lang rather than props.searchParams.lang for App Router pages.

Domain routing

I think domain routing should be possible to define inside middleware.ts.

aralroca commented 1 year ago

Some updates:

  • We should support translation in error, loading and layout components (for Next 13)
  • Next 12 router implements sub-path i18n automatically while translating that to the lang search params at the page level. That's not true for the new Next 13 router. In this regard I think we should follow the pattern provided in the documentation where the user must define a dynamic route to handle sub-path i18n and, after that, we start using the lang param from the dynamic route rather than the search param for the App Router.
  • For Next 13 I think we could leverage best the root layout for loading the namespaces rather than injecting logic on each page. Sure it is done server side, but I think this may be a more elegant solution.

Sub-path routing

Providing more context, in Next 12, when defining i18n options in next.config.js it is possible to navigate to the following routes:

/about loads the default language /en/about loads the en version of the page /br/about loads the br version of the page

For Next 13, users would need to create /app/[lang]/about/page.tsx (same for any other internationalized page). Together with that, users need to create a middleware.ts to redirect to the correct language sub-path. Then we would use page's props.params.lang rather than props.searchParams.lang for App Router pages.

Domain routing

I think domain routing should be possible to define inside middleware.ts.

Yes, the first approach was only for pages. But totally agree, it was pending to implement the rest and improve.

Related with https://github.com/aralroca/next-translate-plugin/issues/7

alvesvin commented 1 year ago

Added a todo list at the top to keep track of the progress 🎉

alvesvin commented 1 year ago

The idea with determineResourceType function is to use it to return a string like APP_ROUTER_SERVER_PAGE, APP_ROUTER_SERVER_LAYOUT, etc, to represent the resource type and based on that we do a switch to add necessary helpers accordingly. Is that ok? I'm afraid to go too far and unnecessarily rewrite too much.

I'm doing this as first step to support translation in layout, error & loading pages.

alvesvin commented 1 year ago

I'm kinda stuck with a weird behavior in layout/loading/error components.

Inside a server layout component the useTranslation hook seems to sometimes get a stale value from the last render and other times it seems to get stuck with one value.

Funny part is that server pages work correctly even though they are being treated exactly the same by the loader (perhaps that's the problem).

I'm going to be full focused on this PR all day tomorrow. Looking forward to finish it.

aralroca commented 1 year ago

@alvesvin Thank you for all your efforts. I have seen that there is currently a bug for hooks/helpers that have the "use client", reported here: https://github.com/aralroca/next-translate-plugin/issues/23

since you are with this complex task could you try to fix this case? otherwise I will try to fix it once your PR is merged. Thanks! 🙏

alvesvin commented 1 year ago

@alvesvin Thank you for all your efforts. I have seen that there is currently a bug for hooks/helpers that have the "use client", reported here: #23

since you are with this complex task could you try to fix this case? otherwise I will try to fix it once your PR is merged. Thanks! pray

Oh, okay. I will fix it.

alvesvin commented 1 year ago

Guys, sorry for the delay. I have only a limited time to work on this PR. This feature is trickier than it looked like, but I'm working on it as much as I can.

I've tried a few things but still haven't come up to a reliable implementation for i18n in layouts, loading & error.

aralroca commented 1 year ago

@alvesvin Don't be afraid to make several PR, I think it would be easier to review. Feels free to remove the part of layout, loading, error and add it in another PR.

alvesvin commented 1 year ago

Hey, @aralroca I'm sorry to say that I think I'm not going to be able to accomplish this PR :disappointed:. Since I use this library in production, I was hoping to try and fix it as it's currently not compatible with Next 13 features. Problem is I think I'm not doing a good job. The last commits is an implementation that feels kinda hacky, bloated and bug-prone. I've tried a few things but I don't have the rationale why the code is the way it is today and what should be done to productively improve it or what should be rethought.

I think I started it wrong, maybe not asking the right questions. If you could shed some light on what you think the solution would be, I'm still willing to implement it.

Sorry to make you all wait for nothing :cactus:

aralroca commented 1 year ago

@alvesvin Don't worry, your contribution has already been important. As soon as I have some time I will try to implement it and I will take into account all your work on this PR. Thank you very much for your contribution 😊

AndresDevelop commented 1 year ago

Brilliant work on this one folks. I was wondering if you have a sort of deadline for the next release. This issues is time sensitive for my project . thank you 🎖️

aralroca commented 1 year ago

@AndresDevelop @alvesvin Try next-translate-plugin 2.3.0-canary.1 prerelease please 🙏

izoukhai commented 1 year ago

Hey @aralroca

Using the canary version gives me this error:

aralroca commented 1 year ago

Hey @aralroca

Using the canary version gives me this error:

  • error ./node_modules/next/dist/compiled/@next/react-refresh-utils/dist/runtime.js

mmmmh! Which version of Next.js are you using @izoukhai?

izoukhai commented 1 year ago

Hey @aralroca Using the canary version gives me this error:

  • error ./node_modules/next/dist/compiled/@next/react-refresh-utils/dist/runtime.js

mmmmh! Which version of Next.js are you using @izoukhai?

Next 13.4.4, along with next-translate 2.0.6.

Forgot to mention that I had no pages/ directory, creating one fixed that issue but raised a new one:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './server.edge' is not defined by "exports" in /Users/izoukhai/test/node_modules/react-dom/package.json

izoukhai commented 1 year ago

Well, after completely restarting the project, it seems to work. However, certain translations get loaded and replaced by their keys quickly after, giving a bunch of error tied to server/client mismatch. I will investigate the issue, might be related to my implementation as it impacts only my navbar

izoukhai commented 1 year ago

@aralroca the issue seems to impact client components only.

aralroca commented 1 year ago

New prerelease https://github.com/aralroca/next-translate-plugin/releases/tag/2.3.0-canary.2

izoukhai commented 1 year ago

New prerelease https://github.com/aralroca/next-translate-plugin/releases/tag/2.3.0-canary.2

The issue with pages folder is gone, but client components still don't load translations

aralroca commented 1 year ago

Thanks for your feedback @izoukhai. I'm going to investigate the client components what change in the last versions of Next.js.

aralroca commented 1 year ago

@izoukhai is possible to create a repo that reproduces this issue to help me to fix it? Thanks!

izoukhai commented 1 year ago

@izoukhai is possible to create a repo that reproduces this issue to help me to fix it? Thanks!

Yup I will give you the repo in a minute!

izoukhai commented 1 year ago

@aralroca here you go: https://github.com/izoukhai/next-translate-example-repo

aralroca commented 1 year ago

Thanks @izoukhai

aralroca commented 1 year ago

@izoukhai thanks for sharing, should be fixed in 2.3.0-canary.3 (next-translate + next-translate-plugin in 2.3.0-canary.3 version)

aralroca commented 1 year ago

Done in 2.3

aralroca commented 1 year ago

@alvesvin @izoukhai Layouts are supported in 2.4.0-canary.2 (next-translate + next-translate-plugin in the same version). Would be great if you can try and give me some feedback before the next release. Thanks 🙏