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
Replacing CLI to Next.js plugin #303
Comments
If someone wants to experiment, I did a first experimental version under Things are still missing, so several things may still fail, but I prefer you to experiment it, and if you find something, report it here (please don't do PR yet to that branch). |
Note: instead of |
@BjoernRave Try with 1.0.0-experimental.2 that now it has some corrections with respect to the |
@aralroca I am getting a |
I used If adding this works: module.exports = {
webpack: (config, { isServer }) => {
// Fixes npm packages that depend on `fs` module
if (!isServer) {
config.node = {
fs: 'empty'
}
}
return config
}
} I will include it inside the same plugin to make it work. Can you confirm it? 😊 and thank you very much for the feedback! |
if you can provide a simple repo reproducing the error, it would be very helpful 🙏 |
That fixed it indeed! Also, check out this comment: vercel/next.js#7755 (comment) EDIT Yep, that comment is right. If I simply include any fs.stat("/a.tsx", () => {}); in my |
However, now I get this error:
You can reproduce my setup here: https://github.com/jan-wilhelm/next-translate-error-reproduction |
Update: I have found that the error appears when using react@16.14.0, but disappears on react@17.0.1 |
Question - Is there any way at all for I wouldn't normally need any of (should this be a new issue?) |
@jan-wilhelm
However, remember that these are default values when the page has no loader. Writing an empty |
@jan-wilhelm Do you think it would make more sense for the fallback for dynamic pages ([slug]) to provide a getStaticPaths with fallback: false? I would love that next-translate uses a getStaticProps by default in this case too, but at the same time I don't like that the translations are loaded in CSR without having the page prerendered |
yeah I totally get that. What would you suggest to do in this scenario? Are we just gonna have to wait for NextJS to support I think personally, for me, I could either go with the I asked the same question on a next-i18next GitHub Issue and I'm curious to see what option they will choose. |
I would say that the easiest way is to use SSR temporarily for these pages (unless you know all the paths). It is the simplest thing, although it has the cost of optimization, we know that it is temporary. If you make temporary alternative routes I think it will be much worse to change it in the future, since you will have to provide a 301 redirect to the new route for each of your products, and you could have serious SEO problems. The i18n support of Next.js right now is only routing, but it is also in their plans to develop a method to load the translations. So this would solve your problem. A little bit the idea to make now the plugin is about the future of i18n inside Next.js. We want that in a future it will be very easy to migrate from one way to another just by upgrading Next.js and next-translate without breaking changes, and gaining then optimization in these pages. I guess and hope that the development about loading the translations from Next.js team will be declarative at the configuration level. It's clear that writing manually in each page which namespaces you want inside a loader (getStaticProps, getServerSideProps) is a pain, and that's why we give the opportunity to define it this way: {
"locales": ["en", "de"],
"defaultLocale": "en",
"pages": {
"*": ["common"], // all pages
"/": ["home"],
"/product/[id]": ["product"],
"rgx:^/account": ["account"], // all pages under /account
"rgx:^/admin": ["admin"] // all pages under /admin
}
} |
Hello, I have tested the Feature Branch 1.0.0 experiment. The setup of my Next Configuration looks like this.
From the configuration you can see how to use Webpack. An essential module contained in the feature supplements the Next-Translate Loader in any detectable module rules, which were created by upstream Webpack steps. In my case these are not only the steps defined by NextJs, but also those that are created in the mentioned merge parts depending on whether development or production. Whether a local or outsourced Webpack configuration is used here is not questionable. The call of dedicated Webpack steps is controlled by next-translate. So projects that have their own Webpack configuration must be called by the call. Adaptation 1:
Adaptation 2: At least one case I could identify, where this expectation does not come true.
What needs to be clarified additionally is how to handle loaders which follow an outdated nesting specification, those where the specification differs from module/rules/ (test|use). Maybe not relevant here.
Question to the overall approach. Is it not enough to do this exactly for Javascript files? Maybe it is possible that someone will validate, improve and experiment with the proposed changes via NPM. Thanks for listening and keep up the cool library! |
@gurkerl83 Thank you very much for your feedback! 👏 About passing the webpack configuration all the way through is something I'm going to fix right now! 😊 Thanks! About webpack 5 support is on the TODO list! heh! In the same file above there are some comments about everything that is still missing. I will take into account everything you tell me to continue developing version 1.0, very useful your comments.
I don't know if I understood the question correctly. You mean that instead of making a The truth is that I also thought about doing it this way, and maybe we can change the way we load it. Basically, I preferred to use this approach because Next.js already adds which files to include and which to exclude, so instead of taking all the But, now you have made me wonder that maybe this is not enough reason. So maybe it makes more sense to add an independent |
@aralroca I saw you made the changes, and provided a new experimental release. Unfortunately it is not working. The reason is that you have initialised the variable use with the loader.
For the webpack options please do not use the spread operator here, there is no reason to do that but makes code more complicated to understand. The wepack interface has two arguments, the first one is config (here conf), the second one is options, options is where isServer lives.
Thx! |
@gurkerl83 thanks for this amazing feedback, I applied the changes under |
Hi @aralroca, with 1.0.0-experimental.10 I get the following compile error when using the
Changing the return type to |
@chrisss404 thank you to report it. I fix it on 1.0.0-experimental.11. I'm taking advantage to migrate the project to TypeScript, so if you see more type errors comment it here or do PR yourself in the 1.0.0-experimental branch! Thanks a lot! |
@aralroca
|
@andrehsu are you getting this error? For this error you should update React to 17 (meanwhile I can't find a way to fix it). If is another error, I would appreciate it very much if you would report what you get. Thank you very much! 🙏 |
@aralroca My IDE inspection gave a warning on |
@andrehsu are you using Webstorm? I see this: https://intellij-support.jetbrains.com/hc/en-us/community/posts/206346419--Method-expression-is-not-of-function-type-error-on-Typescript-constructor-call (I dunno if is related) |
Yes I am, but the issue exists independent of WebStorm. |
@jan-wilhelm it should work with 1.0.0-experimental.15 👍 I've already fixed it, but if I'm honest, I haven't understood why... Let's see if someone can explain it to me! I was debugging and saw that it was something in the code that caused TypeScript to compile badly. So, I was uncommenting line by line, until I saw what was wrong with the line: import App from 'next/app'
// Inside getInitialProps wrapper...
App.getInitialProps() // THIS LINE It seems that when commenting I don't really understand why this could be happening, could it be because we have Next.js as peerDependency? I hope someone can clarify for me why this was happening 🙏 |
The migration guide to 1.0.0 is still available on https://github.com/vinissimus/next-translate/blob/1.0.0-experimental/docs/migration-guide-1.0.0.md (replacing the version to the latest experimental prerelease it should work). Everything is already quite stabilized, in a short time we will surely go from the experimental version to the canary, since it looks like we will finally evolve around here. And I hope that the final version 1.0.0 will be available in December. It would help a lot if you could try the latest prerelease and report possible problems; things in the documentation, issues... 🙏 |
I tested
the url path get's changed and the my i18n.js:
|
@BjoernRave I have copied your By the way, the last experimental one, for now, is the Could it happen on a particular page that you have In +0.19 we are displaying this warning: I see that now this warning is lost, I'll add it again. Do you think it could be that? Thank you! |
ah yes, |
so version |
@BjoernRave the idea is that it works the same as next-translate >= 0.19 where we moved all the routing part of i18n to the way that now the Next.js v10 core has. In Next.js version 10 they support i18n within:
But the support for i18n within getInitialProps on individual pages is missing. I understand that this is a problem, so I opened the issue in the Next.js repo. However, it is something that until it is solved in the Next.js core will not have a solution for now. And understanding that this issue exists since version 0.19 and comes from Next.js core, I guess that version 1.0.0 will leave without this. When there is a solution will only need to upgrade the version of Next.js. Perhaps I will try to contribute in Next.js to try to add this, I suppose that other i18n libraries will also miss it. |
In fact, we have it implemented. If getInitialProps from Next.js sends the locale in its context, then it will work without another release. |
got it, thanks for the clarification and your dedication to make everything work as smooth as possible :) |
Closing this. Already implemented on 1.0.0-canary.1. The 1.0 will be released approximately 1-2 weeks. 😊 |
Now we have changed the way we do the "build". Before it made much more sense, because there was no i18n routing support in Next.js, so what we were doing was creating all possible static pages, working in a parallel directory like
pages_
.This now after 0.19 has evolved in such a way that we only need the build to load the namespaces, so it's time to think of a better alternative.
I have been doing tests in the 1.0.0-experimental branch and I see it very feasible to move this logic to a Next.js plugin, moving the builder part to a webpack loader.
This is going to fix:
pages
directory than Next.js. No need of workarounds.locales
anddefaultLocales
onnext.config.js
, this will be injected by the plugin. So you only will have ai18n.js
ori18n.json
root file.The only change (after renaming
pages_
topages
) will be:Instead of this on package.json:
Use in
next.config.js
file:The idea is to keep the same
i18n.json
/i18n.js
configuration file.The text was updated successfully, but these errors were encountered: