-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
[8.0.0-beta.4] Error when using postProcessors #931
Comments
Somewhat related to #930. I wonder if @filipesmedeiros has an opinion on how we might best avoid all these serialisation errors? |
Hmm the most obvious way is the one I provided on the mr: make the app provide the same config on every place they need it. This would mean every The second option would be to find a way to make everything (including the backend) serializable. This would be great but sounds very difficult, because it means you have to serialize functions. There might be a way to pass the config once to Let me know your thoughts guys! |
Also, don't forget one thing: the setup right now implies that we're almost just sugar for fetching translations with a server instance and passing them to an instance in the client, so maybe it does make sense that the client config is static, with stuff like logic for adding missing translations and whatnot, but let the server be what it is now, which is basically a cached translation fetcher with plugin options. Which is awesome! I don't mean it in a bad way, I think it's exactly what this package should be. |
Hi! My thoughts: First option is solid, and feels like the simple solution even though, as @filipesmedeiros said, it is kind of annoying. Second option may be manageable by implementing a custom Note: function foo() {
const bar = 'something';
const set = new Set();
set.add(bar);
return set;
}
foo.toString() gives us
Not saying this is the key to the second solution but may bring ideas. |
@filipesmedeiros and @LeonardDrs Thanks for good discussion here. Have you seen packages like flatted? Might such a thing help us side-step this issue entirely? |
@isaachinman I don't think You meant packages like stringifier? Found some article which would make the second solution doable, but now I'm feeling that this could be a |
Yeah I agree
I think maybe we should focus on creating a nice/easy way to make config shareable on the app side, and not on ours |
Well unless somehow the module could Edit: import type { AppProps } from "next/app";
import i18nextConfig from "next-i18next.config";
import { appWithTranslation } from "next-i18next";
const App: React.FC<AppProps> = ({ Component, pageProps }) => {
....
};
export default appWithTranslation(i18nextConfig)(App); since the const DEFAULT_CONFIG_PATH = './next-i18next.config.js';
...
if (fs.existsSync(path.resolve(DEFAULT_CONFIG_PATH))) {
userConfig = await import(path.resolve(DEFAULT_CONFIG_PATH));
} so... all good? |
Now that I see this with more attention, why not just make app with translation check the same file? Instead of receiving an object? (or maybe both) |
On the server side, I can see it happening, but on the client-side this need to be in the source and thus, |
True. I was thinking of something like next.config.js, but I think they do webpack magic to pass it to the bundle |
Maybe there is a way for nextjs related packages to plug themselves to the next webpack magic without having to temper the So now the question would be either to have the consumer import its next-i18next config when using |
Yeah. The ugly way would be to make apps expose the i18next config on the public runtime config of next.config and read it on our side with next/config |
In general, this just seems like a serialisation problem to me, and we shouldn't necessarily need to change the API of our software because of that. What about something like Otherwise yes, we could import |
Let me address various point:
|
I tend to agree with you, @filipesmedeiros. Although use of I do not think that maintaining a config file, plus requiring it as an arbitrary argument into a function is a good option. We should have one or the other, so I think I will remove the Also, bear in mind that the Vercel team have been talking about an official plugin system for quite awhile now, with very little movement. Also, I agree with the @LeonardDrs and @filipesmedeiros can you let me know what you think? |
Hello,
I understand and agree but I have some comments about this. It means the user needs to require the config in every file needing
And worst of all, I don't have better solution to propose. |
@LeonardDrs I don't understand this:
I agree with you @isaachinman and I'm curious as to what will happen with Next plugins. To be honest I think the dream is that a framework API is so simple and nice that Plugins don't need a "plugin API". We've kinda seen this with Next, but I guess people are finding more and more complex use cases that it's needed now. |
@filipesmedeiros |
Hey @isaachinman, what are the next steps? Thanks in advance! |
Hey @filipesmedeiros and @LeonardDrs – please checkout #954 and let me know what you think. This is a pretty simple solution, and should allow for an extremely easy user experience. Tim mentioned in this comment that their primary reason for implementing such strict checks on serialisation is to prevent subtle bugs. I don't think i18next configs will run into any gotchas, but I would be very curious if we can get a few people to test this Let me know your thoughts! |
Hey @isaachinman, tested the beta.6 version with the sprintf-postProcessor. I got the error `pageProps._nextI18Next` _nextI18Next: {
initialI18nStore: { 'en-US': [Object] },
initialLocale: 'en-US',
userConfig: '{"i18n":{"locales":["ar-EG","bg-BG","cs-CZ","da-DK","de-DE","en-GB","en-US","es-ES","es-MX","fi-FI","fr-FR","he-IL","hr-HR","hu-HU","id-ID","it-IT","ja-JP","ko-KR","ms
-MY","nb-NO","nl-NL","pl-PL","pt-BR","pt-PT","ro-RO","ru-RU","sk-SK","sl-SI","sq-AL","sr-RS","sv-SE","th-TH","tr-TR","uk-UA","zh-CN"],"defaultLocale":"fr-FR","localePath":"\\u002Fhome\
\u002Fleonarddrs\\u002Fwww\\u002Fstandalone-boilerplate\\u002Flocales","localeStructure":"{{lng}}","nsSeparator":":::","keySeparator":"::","postProcess":"sprintf","use":[{"name":"sprin
tf","type":"postProcessor","process":function process(value, key, options) {\n' +
' if (!options.sprintf) return value;\n' +
'\n' +
" if (Object.prototype.toString.apply(options.sprintf) === '[object Array]') {\n" +
' return (0, _sprintf.vsprintf)(value, options.sprintf);\n' +
" } else if (_typeof(options.sprintf) === 'object') {\n" +
' return (0, _sprintf.sprintf)(value, options.sprintf);\n' +
' }\n' +
'\n' +
' return value;\n' +
' },"overloadTranslationOptionHandler":function overloadTranslationOptionHandler(args) {\n' +
' var values = [];\n' +
'\n' +
' for (var i = 1; i < args.length; i++) {\n' +
' values.push(args[i]);\n' +
' }\n' +
'\n' +
' return {\n' +
" postProcess: 'sprintf',\n" +
' sprintf: values\n' +
' };\n' +
' }}],"ns":"common"},"default":{"i18n":{"locales":["ar-EG","bg-BG","cs-CZ","da-DK","de-DE","en-GB","en-US","es-ES","es-MX","fi-FI","fr-FR","he-IL","hr-HR","hu-HU","id-ID","it-IT","ja-JP","ko-KR","ms-MY","nb-NO","nl-NL","pl-PL","pt-BR","pt-PT","ro-RO","ru-RU","sk-SK","sl-SI","sq-AL","sr-RS","sv-SE","th-TH","tr-TR","uk-UA","zh-CN"],"defaultLocale":"fr-FR","localePath":"\\u002Fhome\\u002Fldrouillas\\u002Fwww\\u002Fstandalone-boilerplate\\u002Flocales","localeStructure":"{{lng}}","nsSeparator":":::","keySeparator":"::","postProcess":"sprintf","use":[{"name":"sprintf","type":"postProcessor","process":function process(value, key, options) {\n' +
' if (!options.sprintf) return value;\n' +
'\n' +
" if (Object.prototype.toString.apply(options.sprintf) === '[object Array]') {\n" +
' return (0, _sprintf.vsprintf)(value, options.sprintf);\n' +
" } else if (_typeof(options.sprintf) === 'object') {\n" +
' return (0, _sprintf.sprintf)(value, options.sprintf);\n' +
' }\n' +
'\n' +
' return value;\n' +
' },"overloadTranslationOptionHandler":function overloadTranslationOptionHandler(args) {\n' +
' var values = [];\n' +
'\n' +
' for (var i = 1; i < args.length; i++) {\n' +
' values.push(args[i]);\n' +
' }\n' +
'\n' +
' return {\n' +
" postProcess: 'sprintf',\n" +
' sprintf: values\n' +
' };\n' +
' }}],"ns":"common"}}}'
}
}
" if (Object.prototype.toString.apply(options.sprintf) === '[object Array]') {\n" +
' return (0, _sprintf.vsprintf)(value, options.sprintf);\n' +
" } else if (_typeof(options.sprintf) === 'object') {\n" +
' return (0, _sprintf.sprintf)(value, options.sprintf);\n' +
' }\n' + but there is no declaration of it anywhere which seems indicate that the imported functions are not serialized as well. Also, really not a big fan of the |
@LeonardDrs Let's consider these as two separate issues:
The second can easily be achieved by just modifying Do you have any suggestions for serialisation? I really would like to avoid having to force users to import and pass the same config around in |
I've been digging and I right now I have no idea how an
I would like to stress out the fact that, unless using a custom backend, the configuration only needs to be passed to |
Sorry, I don't understand how that could be true. Both |
That's because So I rectify, even with a custom backend we only need to pass the user config to |
So I think it comes back to that solution I had on my PR? We give an argument to appWithTranslation to pass the config? |
Your proposed solution is:
Correct? I think we have indeed talked in circles a little bit here, but @LeonardDrs's point about closures and imported dependencies is pretty conclusive. I don't think we're going to find a way around that. |
That's one way. The other way would be to have users put everything inside |
Do we agree that we should use the plugin way if and when it's live? |
I don't see why not, but @isaachinman should have more info to decide on that than me |
How would we do that?
Yes, of course. However, it's been in RFC for quite some time now, and I haven't heard anything promising about movement on that front in any of the private channels. |
If we assume that our config is present inside // appWithTranslation.ts
import getConfig from 'next/config'
const nextI18nextConfig = getConfig().publicRuntimeConfig.next-i18next
const appWithTranslation = () => {
...
createI18nextClient(nextI18nextConfig)
...
}
export appWithTranslation Can we access the users config when we are inside |
Unfortunately,
|
Right, could this live outside |
@filipesmedeiros Unfortunately calling Also, directly importing |
I've just had another thought: What if we simply add a
@filipesmedeiros and @LeonardDrs let me know your thoughts. |
Yeah indeed, best of both worlds. Ready to test the PR anytime. |
@LeonardDrs Yep, and I am very happy to not ship code which uses |
Yeah, makes sense. What do you think is the most sensible option?
Open to any other suggestions as well. |
I suppose we could also do a |
Lastly we could import |
I think it would be best to not serialize userConfig when I'm not not sure I get how we could use Agree for no use of internal. |
That wouldn't work out of the box, because You're right that it doesn't really make much sense to have a I was proposing adding a |
Yep. This or a configuration argument directly in |
Since from what it looks like if we are going to specify it once, we are going to specify it every time |
Ah, that's a nice suggestion. I think I prefer a configuration argument. I've updated the PR – do you mind testing again? |
As soon as I'm available!
…On Wed, 24 Feb 2021, 13:34 Isaac Hinman, ***@***.***> wrote:
Ah, that's a nice suggestion. I think I prefer a configuration argument.
I've updated the PR – do you mind testing again?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#931 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANDZNBC3QGNHVZBJLZDII3TATW4VANCNFSM4W5KRGSA>
.
|
That's perfect. Should we catch the serialize error from next to warn the user about the fact that he needs to set |
It's working as expected for you? I'd rather not catch any errors that NextJs throws. We can just add a section to the Not sure I get your point about jsdoc, can you elaborate? |
Yeah sorry, I should have been more verbose. Should we, in the code, warn the user that we needs to use About JSDocs, when declaring your function like this (ignoring the typing): /**
* Short description
*
* When using the `configOverride` parameter, you may also want to set the
* `serializeConfiguration` option to `false` in the `next-i18next.config.js` file.
*/
const appWithTranslations = (component: any, configOverride: any) => {}; most IDE display the description when hovering functions: |
Yes I'm aware of how jsdoc works, and it would be a fantastic contribution/addition, but I am focused on getting an initial release out as quickly as possible, and will prioritise the main documentation (README). To answer your other point: I'm already throwing an error if the user sets So, for users who add unserialisable data to their config, they will get two paths that lead to errors:
This, in addition to good documentation, should lead users to the right path. |
Yes I've seen this error, this what led me to this thought actually 😄 Documentation is not my strong side at the moment so I'm always afraid about user expectations and whatnot. Anyway, your PR is elegant and perfect for this issue. |
Fixed via a |
Hi, first of all, thanks for this plugin, yours and the community's efforts ❤️
Describe the bug
I'm unable to use an i18next postprocessor - here sprintf-postProcessor-, similar to this comment, any
use
config parameter seems to trigger the following error when trying to start the next application (usingnext
command):Occurs in next-i18next version
next-18next version: 8.0.0-beta.4 - c3dcef1
Steps to reproduce
yarn run-example
Reproducible patch from c3dcef1
Expected behaviour
There should be no errors and I should be able to translate using any backend/plugin
The text was updated successfully, but these errors were encountered: