Skip to content
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

Support App Router & Pages Router #22

Closed
wants to merge 16 commits into from

Conversation

alvesvin
Copy link

@alvesvin alvesvin commented May 17, 2023

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

  • Multiple pages dirs
  • Tests for multiple pages dirs
  • Translation in layouts, error & loading components
  • Tests for translation in layouts, error & loading
  • Next 13 sub-path routing
  • Do not add helpers for custom hooks & hocs
  • Tests for not adding helpers for hooks & hocs

@alvesvin
Copy link
Author

alvesvin commented May 17, 2023

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

@alvesvin alvesvin changed the title Support App Route & Pages Router Support App Router & Pages Router May 17, 2023
src/loader.ts Outdated Show resolved Hide resolved
__tests__/index.test.js Outdated Show resolved Hide resolved
Copy link
Owner

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Keep waiting for the final changes

@alvesvin alvesvin force-pushed the 21-feature-multiple-pages-dir branch from 750ab55 to c5c7c49 Compare May 18, 2023 02:23
@alvesvin
Copy link
Author

alvesvin commented May 18, 2023

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 better 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.

@alvesvin alvesvin requested a review from aralroca May 18, 2023 04:13
@@ -0,0 +1,10 @@
import webpack from 'webpack'

describe('nextTranslate', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

❤️

@aralroca
Copy link
Owner

aralroca commented May 18, 2023

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 #7

@alvesvin
Copy link
Author

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

@alvesvin alvesvin force-pushed the 21-feature-multiple-pages-dir branch from 24abf2f to 5d17fab Compare May 19, 2023 05:01
@alvesvin
Copy link
Author

alvesvin commented May 19, 2023

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
Copy link
Author

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
Copy link
Owner

@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! 🙏

@alvesvin
Copy link
Author

@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
Copy link
Author

alvesvin commented May 24, 2023

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
Copy link
Owner

@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
Copy link
Author

Hey, @aralroca I'm sorry to say that I think I'm not going to be able to accomplish this PR 😞. 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 🌵

@aralroca
Copy link
Owner

@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
Copy link

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
Copy link
Owner

aralroca commented Jun 5, 2023

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

@izoukhai
Copy link

izoukhai commented Jun 6, 2023

Hey @aralroca

Using the canary version gives me this error:

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

@aralroca
Copy link
Owner

aralroca commented Jun 6, 2023

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
Copy link

izoukhai commented Jun 6, 2023

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
Copy link

izoukhai commented Jun 6, 2023

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
Copy link

izoukhai commented Jun 6, 2023

@aralroca the issue seems to impact client components only.

@aralroca
Copy link
Owner

aralroca commented Jun 6, 2023

@izoukhai
Copy link

izoukhai commented Jun 6, 2023

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
Copy link
Owner

aralroca commented Jun 7, 2023

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

@aralroca
Copy link
Owner

aralroca commented Jun 7, 2023

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

@izoukhai
Copy link

izoukhai commented Jun 7, 2023

@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
Copy link

izoukhai commented Jun 7, 2023

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

@aralroca
Copy link
Owner

aralroca commented Jun 7, 2023

Thanks @izoukhai

@aralroca
Copy link
Owner

aralroca commented Jun 8, 2023

@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
Copy link
Owner

Done in 2.3

@aralroca aralroca closed this Jun 18, 2023
@aralroca
Copy link
Owner

aralroca commented Jun 19, 2023

@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 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants