aralroca / next-translate

Next.js plugin + i18n API for Next.js 🌍 - Load page translations and use them in an easy way!
MIT License
2.64k stars 207 forks source link

./i18n.js Critical dependency: the request of a dependency is an expression #851

Closed timheerwagen closed 1 year ago

timheerwagen commented 2 years ago

Today after doing some package updates e.g. next@12.1.6 next-translate crashes while trying to load the locale. Im importing my locales from a package inside my monorepo with:

./i18n.js

loadLocaleFrom: (locale, namespace) =>
    import(`locales/${locale}/${namespace}`).then((m) => m.default),

That worked for me for like 2 months now. But now i get following error:

./i18n.js
Critical dependency: the request of a dependency is an expression

After a while of trial and error i got my code working with:

  loadLocaleFrom: (locale, namespace) =>
    require(`locales/${locale}/${namespace}`),

Is the error here with me or is there a package error?

aralroca commented 2 years ago

If you delete loadLocaleFrom, does it work? By default underneath it makes an identical dynamic import. Looks like an error of the new nextcompiler... Something to investigate. Thanks to report it

rodrigodmpa commented 2 years ago

Today after doing some package updates e.g. next@12.1.6 next-translate crashes while trying to load the locale. Im importing my locales from a package inside my monorepo with:

./i18n.js

loadLocaleFrom: (locale, namespace) =>
    import(`locales/${locale}/${namespace}`).then((m) => m.default),

That worked for me for like 2 months now. But now i get following error:

./i18n.js
Critical dependency: the request of a dependency is an expression

After a while of trial and error i got my code working with:

  loadLocaleFrom: (locale, namespace) =>
    require(`locales/${locale}/${namespace}`),

Is the error here with me or is there a package error?

Same thing here, your solution helped me, thanks.

timheerwagen commented 2 years ago

If you delete loadLocaleFrom, does it work? By default underneath it makes an identical dynamic import. Looks like an error of the new nextcompiler... Something to investigate. Thanks to report it

If i delete loadLocaleFrom i cant load my locales because theyre not in the default folder. Its a package inside my monorepo.

Structure:

monorepo
 |-apps
   |-project
      |-i18n.js
 |-packages
   |-locales
derkoe commented 2 years ago

We have the same problem and also fixed it with require (like https://github.com/vinissimus/next-translate/issues/851#issuecomment-1116090210).

We also had to catch the error on the require because not all of the files exist (we implement a fallback logic for country-specific locales like "es-CL" and then fallback to English):

  const englishTexts = require('./locales/en/myapp.json');
  ...
  loadLocaleFrom: (locale, ns) => {
    let countrySpecific = {},
      languageOnly = {};
    try {
      countrySpecific = require(`locales/${locale.replace('-', '_')}/${ns}.json`);
    } catch (error) {}
    try {
      languageOnly = require(`./locales/${locale.substring(0, 2)}/${ns}.json`);
    } catch (error) {}
    return _.merge({}, englishTexts, languageOnly, countrySpecific);
  },
fabien commented 2 years ago

I'm having the same issue after a redeploy (no code changes, just content publishing) on Vercel. Apparently they made Node v.16 the default since May 9th 2022 - but despite setting v.14 (as engine as well as in settings) it appears not to be the culprit as I initially thought.

Unfortunately, the workarounds shown above don't work for me, since I also depend on tsconfig.json path aliases an their dynamic resolving nature:

loadLocaleFrom: (locale, ns) => {
  return import(`@app/translations/${lang}/${ns}`).then(m => m.default);
}

I tried to circumvent this using tsconfig-paths but this relies on fs eventually, which obviously doesn't work.

Does anyone have more details when and how this 'new' behaviour is triggered? Any way to opt-out if this?

EDIT: the workaround does work, even with require and the path aliases!

derkoe commented 2 years ago

The problem with using require is that all locales will be included in the _app bundle. This makes the first load quite big if you have many languages (as we do). See also #741

derkoe commented 2 years ago

Just looked at the code and saw that the default loader is passed as a string: https://github.com/vinissimus/next-translate/blob/49f580c6d292712a0720a104de3b487e7c11d4ae/src/plugin/utils.ts#L6-L7

I've tried to use a string now in our repo and it works :-) i18n.js:

module.exports = {
  loadLocaleFrom: '(lang, ns) =>  import(`./locales/${lang}/${ns}.json`).catch(e => console.error(e)).then((m) => m.default)',
}

The only issue with that is that you cannot load libs in the string (like lodash).

Update: no it does not - because it checks if the loadLocaleFrom is a function: https://github.com/vinissimus/next-translate/blob/49f580c6d292712a0720a104de3b487e7c11d4ae/src/plugin/index.ts#L83

derkoe commented 2 years ago

When disabling the loader the imports still work - see https://github.com/vinissimus/next-translate/issues/741#issuecomment-1145838941 for an example.

aralroca commented 2 years ago

Yep, I don't recommend require because you are adding more kb to the initial chunk, this was the grace of the dynamic import, to import only what you need.

From what I have seen it fails in the new version of Next.js when the file is from node (it has module.exports). For example, if you put any dynamic import inside any page the same thing happens:

_app.js

import { loadSomethingWithDynamicImport } from '../example'

export default function App(){
  // ...
}

App.getInitialProps = async (context) => {
    const res = await loadSomethingWithDynamicImport('bla')
}

Is working fine whenexample.js is like:

export function loadSomethingWithDynamicImport(param)Β {
  return import(`something/${param}`).then(r => r.default)
}

However it crash with the same error if example.js is like:

function loadSomethingWithDynamicImport(param)Β {
  return import(`something/${param}`).then(r => r.default)
}

module.exports = { loadSomethingWithDynamicImport }

Does anyone have any idea how to solve it without having to use require? Thanks!

aralroca commented 2 years ago

With the default loadLocaleFrom is working fine, the problem in Next.js 12 is where loadLocaleFrom is redefined inside i18n.js because has a module.exports

aralroca commented 2 years ago

Of course, it is not the right solution, but until we find the best way to fix it (all suggestions are welcome), you can do a workaround and not use the loadLocaleFrom inside i18n.js and overwrite the default:

const workaround = require('next-translate/lib/cjs/plugin/utils.js')

// As a workaround you can change the string of the defaultLoader, this is working fine
workaround.defaultLoader = '(l, n) => import(`@next-translate-root/src/locales/${l}/${n}.json`).then(m => m.default)';

module.exports = {
  ...config,
  // loadLocaleFrom: (lang, ns) =>
  //   import(`./src/locales/${lang}/${ns}.json`).then((m) => m.default),
}

Any suggestions from anyone on how to fix it internally in the library? Thanks!

andresz1 commented 2 years ago

The workaround doesn't work for me 😒 any suggestion ? willing to help

mattiaz9 commented 2 years ago

The workaround doesn't work for me 😒 any suggestion ? willing to help

I just use this option:

...
loadLocaleFrom: (lang, namespace) => require(`./lang/${lang}/${namespace}.json`),
...

but I'm having issues with the latest version 1.5, so I had to downgrade to 1.4

aralroca commented 2 years ago

@mattiaz9 What errors do you have in 1.5? Anyway, as I said here and a workaround to solve it here, better not to use require because it will increase your bundle size.

mattiaz9 commented 2 years ago

@aralroca I get this error: TypeError: conf.loadLocaleFrom(...).catch is not a function

mattiaz9 commented 2 years ago

Actually.... I just tested the workaround and it seems to work fine.

This is the correct code:

const workaround = require("next-translate/lib/cjs/plugin/utils.js")

workaround.defaultLoader = "(l, n) => import(`lang/${l}/${n}.json`).then(m => m.default)"

module.exports = {
  // config without 'loadLocaleFrom'
}
aralroca commented 2 years ago

Cool. The ideal is to solve it at the library level without this workaround. I am not very clear on how. It is like a bug (or normal behavior on purpose) of SWC, that crashes the dynamic import only when a file with module.exports has it.

marcusnunes commented 2 years ago

Same problem, I had to downgrade to 1.3.0.

loadLocaleFrom: (locale, namespace) =>
  require(`./src/locales/${locale}/${namespace}`),
aralroca commented 2 years ago

Same problem, I had to downgrade to 1.3.0.

loadLocaleFrom: (locale, namespace) =>
  require(`./src/locales/${locale}/${namespace}`),

Is this workaround not working for you? https://github.com/aralroca/next-translate/issues/851#issuecomment-1173611946 . Better not to use require in order not to increase the bundle size.

itxtoledo commented 2 years ago

same issue here with next 12.2.5

weee-shinan commented 2 years ago

I solved this problem by manually adding the .babelrc file, and using next/babel to process it will not report an error, you can try it @mattiaz9 @fabien @derkoe @andresz1 @marcusnunes

aralroca commented 2 years ago

I solved this problem by manually adding the .babelrc file, and using next/babel to process it will not report an error, you can try it @mattiaz9 @fabien @derkoe @andresz1 @marcusnunes

It's because the bug is added by SWC, by adding the babel you stop using the new Next.js rust compiler. I recommend using the workaround until we find the best solution: https://github.com/aralroca/next-translate/issues/851#issuecomment-1173611946

marcusnunes commented 2 years ago

I just moved my locales from /src/locales to /locales and it worked here.

loadLocaleFrom does not work in the current version

aralroca commented 2 years ago

loadLocaleFrom does not work in the current version

Yep... The new rust compiler of Next.js (SWC) interprets the loadLocaleFrom from i18n.js as server code (because it have a module.export), and SWC doesn't like dynamic imports in server files.

The same error is reproducible without next-translate, creating a file with a dynamic import and module.export:

function loadSomethingWithDynamicImport(param) {
  return import(`something/${param}`).then(r => r.default)
}

module.exports = { loadSomethingWithDynamicImport }`

To solve the problem from NextTranslate we would have to see how we can solve this of the module.export so that SWC interprets correctly the dynamic import, or look for an alternative of how to pass this loadLocaleFrom function. Any proposal will be very welcome since it is a problem that has to be solved but it is not clear to me how we can solve it for now.

donthijackthegit commented 2 years ago

Same here, I have to roll back to next@12.1.0 and problem resolved

donthijackthegit commented 1 year ago

Upgraded to next@12.3.2, need to return promise

loadLocaleFrom: (lang, ns) => {
        // You can use a dynamic import, fetch, whatever. You should
        // return a Promise with the JSON file.
        // yaml already being transformed by webpack plugin
        const m = require(`./locales/${lang}/${ns}.yml`)
        return Promise.resolve(m)
    }
urmauur commented 1 year ago
const m = require(`./locales/${lang}/${ns}.yml`)
        return Promise.resolve(m)

work for me using this πŸ‘ŒπŸ»

felipeferrarini commented 1 year ago

Upgraded to next@12.3.2, need to return promise

loadLocaleFrom: (lang, ns) => {
        // You can use a dynamic import, fetch, whatever. You should
        // return a Promise with the JSON file.
        // yaml already being transformed by webpack plugin
        const m = require(`./locales/${lang}/${ns}.yml`)
        return Promise.resolve(m)
    }

I'm using files in json format and worked too πŸ™Œ

aralroca commented 1 year ago

@felipeferrarini @urmauur @donthijackthegit This issue was introduced after the SWC rust compiler, this compiler doesn't support dynamic import in Node (files with module.export and require).

As a workaround, I recomend this for now

This workaround is avoiding the require, which loads all the namespaces in Webpack instead the only one that you need

aralroca commented 1 year ago

Hello everyone 😊

We are considering making an important change to our library's configuration, where the loadLocaleFrom method currently returns a promise, but in the future it could also return a string.

loadLocaleFrom: (locale, namespace) => `src/translations/${namespace}_${locale}.json`,

With this change, we would be able to use our own Webpack plugin to calculate all available languages and namespaces and place them in separate chunks, thereby improving the performance of the application. This plugin is already implemented in our library and has other functionalities in addition to this. Nevertheless, we will retain the option of returning a promise, to allow users to make fetch or implement their own loading logic.

However, before proceeding with this change, we would like to hear your opinion. How do you think this modification would affect your use of the library? Are there any specific use cases in which this modification could cause problems? Any other suggestions or comments on the matter?

We appreciate your time and welcome any feedback you may provide. ❀️

lukevella commented 1 year ago

Sounds reasonable. Have you considered going a step further and just offer an option to change the root path?

{
   "localeRoot":  "src/translations"
   // equivalent to  loadLocaleFrom: (l, n) => `src/translations/${l}/${n}.json`
}

The library already expects this folder structure out of the box and seems a bit overkill to use loadLocaleFrom when all you need is to specify a different root. loadLocaleFrom can still exist of course for anyone that needs a different structure.

aralroca commented 1 year ago

@lukevella thanks for the comment, I like the idea of keeping things simple, and I had thought about it, although I don't like having two configurations for something similar. πŸ€”

I've seen many cases of people changing the structure of namespaces, either because they support many types of English (UK, US) but many translations are shared and it doesn't make sense to have them duplicated, or namespaces in other formats that are not json, etc... All of these cases should also benefit from having the namespace chunks separated, and that's why I propose the change above.

This issue was introduced because the SWC does not support dynamic import here now and by replacing it with require, it puts all the namespaces in the same chunk, making each page load all the translations instead of just the ones necessary for the page.

With this change, everything is solved; if you want to change the folder, the structure, load it from external resources with a fetch, etc.

lukevella commented 1 year ago

Sure, it would just be great if there wasn't a need to switch from .json to .js for this from a DX perspective. Maybe this can all be handled by having loadLocaleFrom also accept string | () => string.

{
   loadLocaleFrom: "src/translations",
   // equivalent to  loadLocaleFrom: (l, n) => `src/translations/${l}/${n}.json`
}

Either way seems like a step in the right direction as not needing to use dynamic imports in the config would be great for DX.

aralroca commented 1 year ago

I'll keep that in mind, it's a good idea, thank you @lukevella . Maybe we can extend its use to:

string | () => string | () => Promise<json>

aralroca commented 1 year ago

As a workaround, until the new functionality is implemented, if someone wishes to use the require method but have all chunks separated, they can utilize the webpack property provided below to resolve the issue. This sets up a webpack configuration that splits the chunks based on the namespace and locale, so that all chunks are separated and can be easily identified.

const namespaces = ['common', 'dynamic', 'home', 'more-examples', 'error'];
const locales = ['en', 'ca', 'es'];

module.exports = nextTranslate({
  webpack: (config) => {
    namespaces.forEach((namespace) => {
      locales.forEach((locale) => {
        config.optimization.splitChunks = {
          ...config.optimization.splitChunks,
          cacheGroups: {
            ...(config.optimization.splitChunks.cacheGroups || {}),
            [`${namespace}_${locale}`]: {
              test: new RegExp(`src/locales/${locale}/${namespace}.*$`),
              name: `${namespace}_${locale}`,
              chunks: 'all',
              enforce: true,
            },
          }
        }
      })
    })

    return config
  },
})

Then is not a problem using require in i18n.js file:

{
  // ... Rest of config
  loadLocaleFrom: async (locale, namespace) =>
    require(`./src/locales/${locale}/${namespace}.json`),
}
geonwoomun commented 1 year ago

@aralroca This way it is divided into chunks, but loaded all at once on first load. πŸ₯²

aralroca commented 1 year ago

@aralroca This way it is divided into chunks, but loaded all at once on first load. πŸ₯²

wow...! Thanks to detect it @geonwoomun! πŸ˜… So we need to find a solution for this... Do you have some proposals? for now that we are planing to release 2.0.0 we can add a breaking change to fixloadLocaleFrom if is really necessary...

geonwoomun commented 1 year ago

@aralroca I used derkoe's method shown here.

Let me show you part of my code...

_app.tsx

export default appWithI18n(App, {
  ...i18nConfig,
  loadLocaleFrom: (lang, ns) => {
    const langCode = lang ? supportLanguagesMap[lang] : fallbackLanguage;
    return import(`../../public/locales/${location}/${langCode}/${ns}.json`);
  },
});

i18n.js

const i18nConfig = {
  locales: ~~~,
  defaultLocale: 'en',
  pages: {
    '*': ['common'],
  },
  loader: false, // this is required
  interpolation: {
    prefix: '{',
    suffix: '}',
  },
  defaultNS: 'common',
};

By doing this, I confirmed that it works as well as when using babel, is divided into chunks, and does not load all at first.

aralroca commented 1 year ago

@geonwoomun the problem with dynamic imports in SWC is in files with module.exports =, like i18n.js file. Adding in _app.js works fine yes, however we want a solution inside i18n.js. This file is read inside the next.config.js (using our webpack plugin).

kdy1 commented 1 year ago

I don't think it's a bug of swc. I'm not sure if I got your sentence, though.

geonwoomun commented 1 year ago

@aralroca I asked swc maintainer kdy and he answered above.

aralroca commented 1 year ago

@kdy1 A summary of this issue:

In previous versions of Next.js prior to version 12, dynamic import in the configuration worked correctly. It allowed next-translate users to choose where they wanted to have the translation files and only download the chunk that corresponds to the current page. For example, if the page was in English, only the English translations would be loaded, not all languages.

This was possible because the configuration was read within the Webpack plugin that we have, which is executed within the next.config.js file, and during the parsing of the files, the import was added in the loader of each page (getStaticProps, etc.) so that users wouldn't have to do it manually on each page.

However, this stopped working correctly in versions equal to or greater than Next.js 12, and the following error occurred using dynamic import:

Critical dependency: the request of a dependency is an expression

It worked with Babel because it was supported with this plugin, which suggests that it is not supported with SWC.

Starting from version 12, next-translate users have to replace the dynamic import with require.

-return import(`./locales/${langCode}/${ns}.json`)
+return require(`./locales/${langCode}/${ns}.json`)

It loads the translations and the application works fine... but unlike before, it loads all the translations instead of just the one that the page needs.

Since then, we have been looking for a way to solve this at the configuration level, to find an alternative to the dynamic import and require, and to resolve this in an easy way for users...

Of course, the most practical solution would be for dynamic import to continue working. After researching, dynamic import only works with files with ECMAScript 6 Modules, but with files such as CommonJS it does not. Moving this configuration logic directly to the page (ECMAScript 6 modules) does work for dynamic import.

kdy1 commented 1 year ago

Dynamic import is a standard and it's supported by swc

kdy1 commented 1 year ago

AFAIK there's auto_cjs pass which changes module mode to commonjs based on the content. Can you file an issue?

kdy1 commented 1 year ago

Nevermind, I'll use https://github.com/vercel/next.js/issues/40231

aralroca commented 1 year ago

@kdy1 thanks for this PR https://github.com/vercel/next.js/pull/45836 ! Great news πŸŽ‰πŸ₯³

This issue is already fixed in Next.js 13.1.7-canary.11 without any change on Next-translate configuration, you can still use dynamic import inside πŸ‘ 😊

laymonage commented 1 year ago

For anyone else upgrading to next-translate@2 but still stuck on Next.js 12 (e.g. because Preact is not yet supported by Next.js 13), you need to make some changes to the workaround mentioned in https://github.com/aralroca/next-translate/issues/851#issuecomment-1173611946:

- const workaround = require('next-translate/lib/cjs/plugin/utils.js')
+ const workaround = require('next-translate-plugin/lib/cjs/utils.js');

And the workaround should not be placed in i18n.js, because the plugin's utils.js now imports TypeScript. If you put the workaround in i18n.js, TypeScript will be included in your application bundle, massively increasing its size.

I placed it directly in next.config.js and it seems to work fine, though I did have to extract some of the data needed for the defaultLoader workaround string into a separate file.

Glad to hear it's been fixed in Next.js, though! Thanks to everyone involved πŸ˜„

mattvb91 commented 1 year ago

so ive finally managed to get my __namespace loading with the solutions from above, but the only reason I jumped through the hoops trying to get this working is because I need to manually set the language through getInitialProps, which I now cant do anymore because it uses the locale coming from appWithI18n. Does anyone have any idea how to manually change the lang from inside a global _app getInitialProps?

Macumba45 commented 1 year ago

Hello guys,

I cant find a solution...

I have nextJs 13 and next-translate-plugin.

In local works perfect but when i upload to VercelΒ΄s server, the translations does not works...

Do you know what could be? I am stucked more than 2 days and i am lost already....

Thanks!!!