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

[8.0.0-beta.4] Error when using postProcessors #931

Closed
LeonardDrs opened this issue Feb 1, 2021 · 55 comments
Closed

[8.0.0-beta.4] Error when using postProcessors #931

LeonardDrs opened this issue Feb 1, 2021 · 55 comments

Comments

@LeonardDrs
Copy link
Contributor

LeonardDrs commented Feb 1, 2021

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 (using next command):

SerializableError: Error serializing ._nextI18Next.userConfig.use[0].process returned from getStaticProps in "/".
Reason: function cannot be serialized as JSON. Please only return JSON serializable data types.

Occurs in next-i18next version

next-18next version: 8.0.0-beta.4 - c3dcef1

Steps to reproduce

  • Checkout c3dcef1 in this repository
  • Apply "Reproducible patch" bellow
  • Run yarn run-example
Reproducible patch from c3dcef1
diff --git a/examples/simple/next-i18next.config.js b/examples/simple/next-i18next.config.js
new file mode 100644
index 0000000..5366457
--- /dev/null
+++ b/examples/simple/next-i18next.config.js
@@ -0,0 +1,6 @@
+const sprintf = require("i18next-sprintf-postprocessor");
+
+module.exports = {
+  postProcess: "sprintf",
+  use: [sprintf],
+};
diff --git a/examples/simple/package.json b/examples/simple/package.json
index be9e52c..83c434f 100644
--- a/examples/simple/package.json
+++ b/examples/simple/package.json
@@ -10,6 +10,7 @@
   },
   "dependencies": {
     "next": "^10.0.0",
+    "i18next-sprintf-postprocessor": "latest",
     "next-i18next": "link:../../"
   },
   "devDependencies": {

Expected behaviour

There should be no errors and I should be able to translate using any backend/plugin

@isaachinman
Copy link
Contributor

Somewhat related to #930. I wonder if @filipesmedeiros has an opinion on how we might best avoid all these serialisation errors?

@filipesmedeiros
Copy link
Contributor

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 serverSideTranslations and appWithTranslation. Of course this is not as bad as it sounds cuz you can just create a config object once somewhere (utils or whatever) and reuse it everywhere. It's still not very maintainable and kind of annoying

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 i18n and let it manage server/client side stuff, but since we're creating a separate client for each of them I don't if this is possible.

Let me know your thoughts guys!

@filipesmedeiros
Copy link
Contributor

filipesmedeiros commented Feb 1, 2021

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.

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 1, 2021

Hi!
Sorry didn't find the related, and on point, PR!

My thoughts:

First option is solid, and feels like the simple solution even though, as @filipesmedeiros said, it is kind of annoying.
But I like it anyway because it avoids passing the full config in props.

Second option may be manageable by implementing a custom toJSON method on each non-serializable config parameters, and then parsing it when doing the createClient method.
I'm feeling a bit uneasy about the responsibility this package would hold as it could lead to unexpected behavior on third party modules, maybe?

Note: .toString() on a function returns an eval-able string.
e.g:

function foo() {
    const bar = 'something';
    const set = new Set();
    set.add(bar);
    return set;
}

foo.toString()

gives us

"function foo() {
    const bar = 'something';
    const set = new Set();
    set.add(bar);
    return set;
}"

Not saying this is the key to the second solution but may bring ideas.

@isaachinman
Copy link
Contributor

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

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 3, 2021

@isaachinman I don't think flatted can parse function, tried it below:
image

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 Next responsibility since they are the ones serializing props, but I don't know if they would be keen on implementing it. Especialy since it may be wrong to share instances/functions this way?

@filipesmedeiros
Copy link
Contributor

Yeah I agree

but now I'm feeling that this could be a Next responsibility since they are the ones serializing props, but I don't know if they would be keen on implementing it. Especialy since it may be wrong to share instances/functions this way?

I think maybe we should focus on creating a nice/easy way to make config shareable on the app side, and not on ours

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 3, 2021

Well unless somehow the module could import ../../next-i18next.config.js in appWithTranslations and serverSideTranslations, I feel like your pr #930 is the way to go.

Edit:
Actually, unless I'm missing something @filipesmedeiros, your PR only require the consumer to pass the config (if any) once, in _app.tsx like so:

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 serverSideTranslations already requires the userConfig:

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?

@filipesmedeiros
Copy link
Contributor

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)

@LeonardDrs
Copy link
Contributor Author

On the server side, I can see it happening, but on the client-side this need to be in the source and thus, import-ed by the consumer, no?

@filipesmedeiros
Copy link
Contributor

True. I was thinking of something like next.config.js, but I think they do webpack magic to pass it to the bundle

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 5, 2021

Maybe there is a way for nextjs related packages to plug themselves to the next webpack magic without having to temper the next.config.js ? But this it out of my league for now.

So now the question would be either to have the consumer import its next-i18next config when using appWithTranslation or trying some yet-unknown stuff to expose a decorated webpack config - which would import the next-i18next.config.js file - to include in the next.config.js file, ala withPlugins

@filipesmedeiros
Copy link
Contributor

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

@isaachinman
Copy link
Contributor

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 serialize-javascript? Use of eval is a little scary, but I think it'd get the job done.

Otherwise yes, we could import next-i18next.config and pipe it into appWithTranslation, but what is the good of a .config file if you still have to import it and pass it around?

@filipesmedeiros
Copy link
Contributor

filipesmedeiros commented Feb 6, 2021

Let me address various point:

  1. a .config file would be best if you didn't have to import it, I agree, but in some case it serves as just a source of truth, like a constant (see next.config.js where you can read it with getConfig from next/config and use the values inside the code itself). In this case, I'd love for it to go automatically to every instance of options we need, and we can do that using next.config.js (asking users to add it there) This is a Next "plugin" after all.
  2. I don't love using eval trickery, but hey, not against it if it works
  3. I much prefer the idea of creating a const NEXT_I18NEXT_CONFIG in shared/constants (or wherever the user wants) and just using it both in appWithTranslations and serverSideTranslations in app land.
  4. Really off-topic, but as long as I have you here @isaachinman : can you consider renaming serverSideTranslations to getServerSideTranslations before closing the v8 API? I feel like it would be cool if the naming could adhere more closely to that of Next's. This is just a detail, though

@isaachinman
Copy link
Contributor

I tend to agree with you, @filipesmedeiros.

Although use of eval may solve the problem here, it also occurs to me that users sometimes want to be able to flip their next-i18next config based on env (Node vs client). If we serialised the config object in serverSideTranslations, that would no longer be possible.

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 next-i18next.config.js convention.

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 getServerSideTranslations naming suggestion.

@LeonardDrs and @filipesmedeiros can you let me know what you think?

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 7, 2021

Hello,

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 next-i18next.config.js convention.

I understand and agree but I have some comments about this.

It means the user needs to require the config in every file needing getServerSideTranslations and appWithTranslations versus having to import it only in appWithTranslations. It's still okay as the user can decorate the getServerSideTranslations to leverage the import issue, if needs be.

It bothers me a bit that every pages are going to require the configuration solely for the purpose of server/build-side.
The client-side page.js file will have a "useless" import of a configuration object.
Unless we specify that the config import/require needs to be located in the getStaticProps/getServerSideProps functions?
Perhaps I'm just overstating this issue though.
Never mind, not an issue at all.

And worst of all, I don't have better solution to propose.
So I'd say go for it until we can plug ourselves to Next.js with the RFC you mentionned

@filipesmedeiros
Copy link
Contributor

@LeonardDrs I don't understand this:

It bothers me a bit that every pages are going to require the configuration solely for the purpose of server/build-side.
The client-side page.js file will have a "useless" import of a configuration object.
Unless we specify that the config import/require needs to be located in the getStaticProps/getServerSideProps functions?

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.

@LeonardDrs
Copy link
Contributor Author

@filipesmedeiros
Okay tried to explain it with actual code but this doesn't happen so.. never mind. The text you quoted can be ignored.

@LeonardDrs
Copy link
Contributor Author

Hey @isaachinman, what are the next steps?
Should @filipesmedeiros update the PR #930?
Or are you going to open a new one?

Thanks in advance!

@isaachinman
Copy link
Contributor

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 serialize-javascript implementation.

Let me know your thoughts!

@isaachinman isaachinman mentioned this issue Feb 21, 2021
@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 22, 2021

Hey @isaachinman, tested the beta.6 version with the sprintf-postProcessor.
It would seem that serialize-javascript does not handle closure/imports that well.

I got the error _sprintf is not defined and when I look at the pageProps I have:

`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"}}}'
  }
}

_sprintf is used in

      "    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 localePath being present in my production code since props are visible in the generated html.

@isaachinman
Copy link
Contributor

@LeonardDrs Let's consider these as two separate issues:

  1. Closures and imports in regards to serialisation
  2. Exposing localePath client side

The second can easily be achieved by just modifying localePath to the client version inside serverSideProps instead of inside createClient.

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 _app and all pages. However, if there really aren't any other sensible solutions, I can easily modify the API.

@LeonardDrs
Copy link
Contributor Author

I've been digging and I right now I have no idea how an import could be parsed. Especially since bundlers often have their own scope, I feel like _sprintf's content would be forever lost.

I really would like to avoid having to force users to import and pass the same config around in _app and all pages.

I would like to stress out the fact that, unless using a custom backend, the configuration only needs to be passed to appWithTranslations.
If we use a custom backend, it would indeed force user to pass the same config to every serverSideTranslations call since serverSideTranslations only job is to retrieve translations.

@isaachinman
Copy link
Contributor

I would like to stress out the fact that, unless using a custom backend, the configuration only needs to be passed to appWithTranslations.

Sorry, I don't understand how that could be true. Both serverSideTranslations and appWithTranslation need full access to the user config.

@LeonardDrs
Copy link
Contributor Author

That's because serverSideTranslations already imports the config:
https://github.com/isaachinman/next-i18next/blob/next-v10/src/serverSideTranslations.ts#L24

So I rectify, even with a custom backend we only need to pass the user config to appWithTranslations.

@filipesmedeiros
Copy link
Contributor

So I think it comes back to that solution I had on my PR? We give an argument to appWithTranslation to pass the config?

@isaachinman
Copy link
Contributor

Your proposed solution is:

  1. Maintain a convention of next-i18next.config.js file
  2. Continue to import next-i18next.config.js file inside serverSideTranslations to save users some trouble
  3. Require users to import their next-i18next.config.js file in _app and pass as an argument.

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.

@filipesmedeiros
Copy link
Contributor

That's one way. The other way would be to have users put everything inside next.config.js (if they wanna have a next-i18next.config.js and import it there it's fine, but not required), and then we can rely on that and pull that on both ends through next/config.

@LeonardDrs
Copy link
Contributor Author

Also, bear in mind that the Vercel team have been talking about an official plugin system for quite awhile now, with very little movement.

Do we agree that we should use the plugin way if and when it's live?

@filipesmedeiros
Copy link
Contributor

I don't see why not, but @isaachinman should have more info to decide on that than me

@isaachinman
Copy link
Contributor

and then we can rely on that and pull that on both ends through next/config

How would we do that?

Do we agree that we should use the plugin way if and when it's live?

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.

@filipesmedeiros
Copy link
Contributor

If we assume that our config is present inside next.config.js, we can use it in our code, like:

// 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 node_modules? I think so, but not sure

@isaachinman
Copy link
Contributor

Unfortunately, publicRuntimeConfig has performance implications and cannot be considered. Please check the docs:

A page that relies on publicRuntimeConfig must use getInitialProps to opt-out of Automatic Static Optimization. Runtime configuration won't be available to any page (or component in a page) without getInitialProps.

@filipesmedeiros
Copy link
Contributor

filipesmedeiros commented Feb 22, 2021

Right, could this live outside runtimeConfig? So just: getConfig().nextI18next. That way Next can optimize it. And it is, after all, a static configuration, not a runtime one, right?

@isaachinman
Copy link
Contributor

@filipesmedeiros Unfortunately calling getConfig() from a dependency results in undefined. I don't think we have a graceful way to support this until the Vercel team actually support plugins in a first class way.

Also, directly importing next.config.js from the filesystem isn't an option either, as it doesn't exist in Vercel deploys (for example) and cannot be read.

@isaachinman
Copy link
Contributor

I've just had another thought: serverSideTranslations already supports a configOverride via the third argument.

What if we simply add a configOverride argument to appWithTranslation, as well? We should be able to:

  1. Remove serialize-javascript
  2. Still support the majority of users who don't rely on plugins
  3. Allow "power users" to pass whatever config they want into both functions

@filipesmedeiros and @LeonardDrs let me know your thoughts.

@LeonardDrs
Copy link
Contributor Author

Yeah indeed, best of both worlds. Ready to test the PR anytime.

@isaachinman
Copy link
Contributor

@LeonardDrs Yep, and I am very happy to not ship code which uses eval/Function. PR is linked above, can you please review and test?

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 24, 2021

getStaticProps/getServerSideProps still fail when using serverSideTranslations when trying to serialize the configuration:

Error: Error serializing `._nextI18Next.userConfig.use[0].process` returned from `getStaticProps` in "/pre-generated".
Reason: `function` cannot be serialized as JSON. Please only return JSON serializable data types.
    at isSerializable (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:9:7)
    at Object.entries.every (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:503)
    at Array.every (<anonymous>)
    at isSerializable (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:304)
    at value.every (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:864)
    at Array.every (<anonymous>)
    at isSerializable (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:768)
    at Object.entries.every (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:503)
    at Array.every (<anonymous>)
    at isSerializable (/home/ldrouillas/www/standalone-boilerplate/node_modules/next/dist/lib/is-serializable-props.js:7:304)

getStaticProps

export const getStaticProps: GetStaticProps = async ({
  locale,
  locales,
}): ReturnType<GetStaticProps> => {
  return {
    props: {
      locale,
      locales,
      ...(await serverSideTranslations(locale!, ["common"])),
    },
  };
};

_app export

export default appWithTranslation(App, configNextI18Next);

and just to be certain with a console.log('configOverride', configOverride); in appWithTranslations
image

Tried opting out use from _nextI18Next.userConfig in serverSideTranslations:

  userConfig.use = [];
  userConfig.default.use = [];
  return {
    _nextI18Next: {
      initialI18nStore,
      initialLocale,
      userConfig,
    },

but then it only works because my use usage is not one who tempers with translation fetching in node.
If the power user needs a custom backend, I suspect it won't work.

@isaachinman
Copy link
Contributor

Yeah, makes sense. What do you think is the most sensible option?

  1. Do not serialise userConfig when configOverride is passed into serverSideTranslations, and throw an error/warning in appWithTranslation if serialised config is missing, and configOverride is also missing
  2. Add an explicit serializeConfig argument to serverSideTranslations

Open to any other suggestions as well.

@isaachinman
Copy link
Contributor

I suppose we could also do a JSON.parse(JSON.stringify(config)) in a try/catch, but that may result in unexpected and non-obvious behaviour for users (missing and broken plugins).

@isaachinman
Copy link
Contributor

Lastly we could import is-serializable-props from NextJs, but I would really rather not rely on undocumented internals, as they could change at any point.

@LeonardDrs
Copy link
Contributor Author

I think it would be best to not serialize userConfig when configOverride is passed into appWithTranslation since we do not particulary need configOverride in serverSideTranslations (I have no idea how configOverride would be used in serverSideTranslations)

I'm not not sure I get how we could use serializeConfig in serverSideTranslations, care to develop?

Agree for no use of internal.
I'll lunch over it and let you know if I got anything.

@isaachinman
Copy link
Contributor

That wouldn't work out of the box, because serverSideTranslations has no idea what arguments appWithTranslation is going to be called with.

You're right that it doesn't really make much sense to have a configOverride argument on serverSideTranslations, because we can always just read from the filesystem.

I was proposing adding a serializeConfig: boolean argument to serverSideTranslations to control the behaviour. Perhaps we could swap the configOverride: UserConfig argument in serverSideTranslations with serializeConfig: boolean?

@LeonardDrs
Copy link
Contributor Author

Yep. This or a configuration argument directly in next-i18next.config.js

@LeonardDrs
Copy link
Contributor Author

Since from what it looks like if we are going to specify it once, we are going to specify it every time

@isaachinman
Copy link
Contributor

Ah, that's a nice suggestion. I think I prefer a configuration argument.

I've updated the PR – do you mind testing again?

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 24, 2021 via email

@LeonardDrs
Copy link
Contributor Author

That's perfect.

Should we catch the serialize error from next to warn the user about the fact that he needs to set serializeConfig: false in the configuration?
Or notice the user in the jsdoc of the configOverride parameter in the function appWithTranslation about this configuration option?

@isaachinman
Copy link
Contributor

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 next-i18next docs about usage with non-serialisable configs.

Not sure I get your point about jsdoc, can you elaborate?

@LeonardDrs
Copy link
Contributor Author

LeonardDrs commented Feb 24, 2021

Yeah sorry, I should have been more verbose.
It works perfectly for me.
Yeah I felt it would be a bad idea to catch any next error. I should have worded my thought differently:

Should we, in the code, warn the user that we needs to use serializeConfig config option when specifying himself the configuration in appWithTranslation?
It may be a moot point because it will be already specified in the documentation.

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:

image

@isaachinman
Copy link
Contributor

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 serializeConfig to false but doesn't pass a configOverride.

So, for users who add unserialisable data to their config, they will get two paths that lead to errors:

  1. NextJs throws serialisation error
  2. next-i18next throws missing config error

This, in addition to good documentation, should lead users to the right path.

@LeonardDrs
Copy link
Contributor Author

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.
I agree, let's focus on the initial release, I'm rambling sorry.

Anyway, your PR is elegant and perfect for this issue.

@isaachinman
Copy link
Contributor

Fixed via a serializeConfig option in #972.

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

No branches or pull requests

3 participants