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

Replacing CLI to Next.js plugin #303

Closed
aralroca opened this issue Nov 3, 2020 · 36 comments
Closed

Replacing CLI to Next.js plugin #303

aralroca opened this issue Nov 3, 2020 · 36 comments
Assignees
Milestone

Comments

@aralroca
Copy link
Owner

aralroca commented Nov 3, 2020

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.

proposal

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:

  • Hotreloading/refresh files
  • Faster build
  • De-duping logic. Right now there are some alternatives: with builder, without builder + appWithI18n, etc. This can be all integrated inside the plugin, skipping the webpack loader or not.
  • Working in the same pages directory than Next.js. No need of workarounds.
  • Easy to test
  • Easy to migrate in the future
  • No need to indicate to the Next.js config the locales and defaultLocales on next.config.js, this will be injected by the plugin. So you only will have a i18n.js or i18n.json root file.

The only change (after renaming pages_ to pages) will be:

Instead of this on package.json:

{
  "scripts": {
    "dev": "next-translate && next dev",
    "build": "next-translate && next build",
  }
}

Use in next.config.js file:

const nextTranslate = require('next-translate')

//...

module.exports = nextTranslate(nextConfig)

The idea is to keep the same i18n.json / i18n.js configuration file.

@aralroca aralroca self-assigned this Nov 3, 2020
@aralroca aralroca added this to the 1.0.0 milestone Nov 3, 2020
@aralroca
Copy link
Owner Author

aralroca commented Nov 3, 2020

If someone wants to experiment, I did a first experimental version under 1.0.0-experimental.1 prerelease.

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

@aralroca
Copy link
Owner Author

aralroca commented Nov 3, 2020

Note: instead of localesPath, now is loadLocaleFrom (already in the doc) for all the cases: build step, custom server, etc. By default is getting the files from locales folder as before.

@BjoernRave
Copy link
Contributor

BjoernRave commented Nov 5, 2020

When trying to load a normal page I get the following error in the console:
Screenshot 2020-11-05 at 12 49 11

When trying to load a page with getInitialProps I get:

error - ./node_modules/next-translate/index.js:1:833
Module not found: Can't resolve 'fs'
null
TypeError: Cannot read property 'prototype' of undefined
    at loadGetInitialProps (/Users/bjoern/projects/inventhora/page-builder/node_modules/next/dist/next-server/lib/utils.js:3:690)
    at appGetInitialProps (/Users/bjoern/projects/inventhora/page-builder/node_modules/next/dist/pages/_app.js:4:106)
    at /Users/bjoern/projects/inventhora/page-builder/node_modules/next-translate/appWithI18n.js:1:2454
    at tryCatch (/Users/bjoern/projects/inventhora/page-builder/node_modules/regenerator-runtime/runtime.js:63:40)
    at Generator.invoke [as _invoke] (/Users/bjoern/projects/inventhora/page-builder/node_modules/regenerator-runtime/runtime.js:293:22)
    at Generator.next (/Users/bjoern/projects/inventhora/page-builder/node_modules/regenerator-runtime/runtime.js:118:21)
    at asyncGeneratorStep (/Users/bjoern/projects/inventhora/page-builder/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
    at _next (/Users/bjoern/projects/inventhora/page-builder/node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)
    at /Users/bjoern/projects/inventhora/page-builder/node_modules/@babel/runtime/helpers/asyncToGenerator.js:32:7
    at new Promise (<anonymous>)
Could not find files for /project in .next/build-manifest.json

Not sure if I configured it right, my config:

//i18n.js

module.exports = {
  locales: ['en', 'es', 'de', 'pt'],
  defaultLocale: 'en',
  loadLocaleFrom: (lang, ns) =>
    import(`./locales/${lang}/${ns}.json`).then((m) => m.default),
  pages: {
    '*': ['common'],
  },
}
//next.config.js

require('dotenv').config()
const withI18n = require('next-translate')

module.exports = withI18n({
  env: {
    BASE_URL: process.env.BASE_URL,
  },
})

and that's it, right?

@aralroca
Copy link
Owner Author

aralroca commented Nov 5, 2020

@BjoernRave Try with 1.0.0-experimental.2 that now it has some corrections with respect to the experimental.1. If it doesn't work either, you would paste here your code of your _app.js file? 😊 Thank you!

@jan-wilhelm
Copy link

@aralroca I am getting a Module not found: Error: Can't resolve 'fs error when using 1.0.0-experimental.2. I am using TypeScript, NextJS 10, React 16, a very simple babel file... what further information would you need to help me pinpoint this?

@aralroca
Copy link
Owner Author

aralroca commented Nov 5, 2020

I usedfs inside the plugin to read the configuration file (i18n.json). I don't have any problem with it, but maybe it could be related to this vercel/next.js#7755 (comment) ?

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!

@aralroca
Copy link
Owner Author

aralroca commented Nov 5, 2020

if you can provide a simple repo reproducing the error, it would be very helpful 🙏

@jan-wilhelm
Copy link

jan-wilhelm commented Nov 6, 2020

That fixed it indeed! Also, check out this comment: vercel/next.js#7755 (comment)
That might be the solution?

EDIT

Yep, that comment is right. If I simply include any fs call, such as

fs.stat("/a.tsx", () => {});

in my getStaticProps method, then NextJS' babel plugin removes that import from the browser plugin and that error disappears

@jan-wilhelm
Copy link

However, now I get this error:

Error was not caught ReferenceError: exports is not defined
    at Module.eval (webpack-internal:///./node_modules/next/dist/pages/_app.js:8)
    at eval (webpack-internal:///./node_modules/next/dist/pages/_app.js:108)

You can reproduce my setup here: https://github.com/jan-wilhelm/next-translate-error-reproduction

@jan-wilhelm
Copy link

Update: I have found that the error appears when using react@16.14.0, but disappears on react@17.0.1

@jan-wilhelm
Copy link

jan-wilhelm commented Nov 8, 2020

Question - Is there any way at all for next-translate not to use getServerSideProps? I am trying to set up a NextJS application where we have a route like products/[productId]. I do not know all possible productIds at build time, as they can constantly change. However, I want that ProductID-Page to be statically prerendered and to use next-translate. NextJS currently forces me to export all possible getStaticPaths.
Apparently, next-translate currently circumvents this by using getServerSideProps which disables automatic static optimisation. However, the loading screen for each productID is always the same and content is only fetched on the client side, so theoretically NextJS should just be able to pre-generate the loading screen for all any productID.

I wouldn't normally need any of getStaticProps, getStaticPaths or getServerSideProps at all on this page, however next-translate currently forces me to use SSR :/

(should this be a new issue?)

@aralroca
Copy link
Owner Author

aralroca commented Nov 9, 2020

@jan-wilhelm next-translate now uses the existing loaders if the page already has one. If your page has a getStaticProps, it will use a getStaticPropsto load the translations. The problem is with the default values, when there is no existing loader. In this case, there are 3 cases:

  • The normal case: Always use a getStaticProps to have maximum performance.
  • For a dynamic page (example/[slug] or example/[...catchall]), it's using getServerSideProps because it is required by Next.js that if you want to provide the getStaticProps you must also write the getStaticPaths, and we have no idea of the paths that will be used, and to avoid putting a "fallback: false", we decided that perhaps, in this case, it was better that the fallback was the getServerSideProps.
  • When the page uses an external HOC it is using getInitialProps by default to avoid possible conflicts.

However, remember that these are default values when the page has no loader. Writing an empty getStaticProps will cause next-translate to use getStaticProps to load the translations.

@aralroca
Copy link
Owner Author

aralroca commented Nov 9, 2020

@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

@jan-wilhelm
Copy link

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 getStaticProps without getStaticPaths? It feels wasteful to me to always render the same loading screen skeleton over and over again, because I do not want to use SSR.

I think personally, for me, I could either go with the fallback: true approach or make it so that the page is not dynamic (something like products/?id=PRODUCT_ID so that it can get pre-rendered. I think it is fine for next-translate to keep using getServerSideProps for this by default.

I asked the same question on a next-i18next GitHub Issue and I'm curious to see what option they will choose.

@aralroca
Copy link
Owner Author

aralroca commented Nov 9, 2020

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

@gurkerl83
Copy link
Contributor

gurkerl83 commented Nov 10, 2020

Hello, I have tested the Feature Branch 1.0.0 experiment. The setup of my Next Configuration looks like this.

const { merge } = require('webpack-merge');
const nextTranslate = require("next-translate");
const nextConfig = {
	// Called within next-Translate, criteria the property webpack a function
  webpack: (config, options) => {
    return merge(
      config,
      process.env.NODE_ENV === 'development
        ? webpackConfigDev(config, options)
        : webpackConfigProd(config, options)
    );
  },
module.exports = nextTranslate(nextConfig)

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:
In NextJs there is a request to block the FS if !server

https://github.com/vinissimus/next-translate/blob/c2579f6d96df6bc4add109a916fa44e363851087/src/index.js#L58

webpack(conf) {
	const config =
		typeof nextConfig.webpack === 'function
		? nextConfig.webpack(conf)
		: conf
webpack(conf, options) {
	const config =
		typeof nextConfig.webpack === 'function
		? nextConfig.webpack(conf, options)
		: conf

Adaptation 2:
The Branch 1.0.0-experimental.2 is not compatible to Webpack 5. The existing code creates undefined values in the array if "use". The existing code distinguishes two variants, if array or not. If the variable is not an array it is expected to be an object.

At least one case I could identify, where this expectation does not come true.

Input:
{ test: /\.m?js/, resolve: { fullySpecified: false } },

Output:
{ test: /\.m?js/, resolve: { fullySpecified: false }, use: [] },

https://github.com/vinissimus/next-translate/blob/c2579f6d96df6bc4add109a916fa44e363851087/src/index.js#L82

const use = Array.isArray(r.use) ? [...r.use, loader] : [r.use, loader]
let use = [];
if(array.isArray(r.use)) {
	use = [...r.use, loader];
} else if (typeof r.use === 'object') {
	use = [r.use, loader]
} else if ( r !== null && r.use === null) {
	use = [{...r}, loader]
}

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.

module.loaders were deprecated since Webpack 2 and are now removed in favour of module.rules.

Question to the overall approach.
As seen in the last step, all loaders are mutated exactly on the first pass. My question is what exactly should be achieved when this is done across all rules, to mount the next-translate Loader.

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!

@aralroca
Copy link
Owner Author

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

Question to the overall approach.
As seen in the last step, all loaders are mutated exactly on the first pass. My question is what exactly should be achieved when this is done across all rules, to mount the next-translate Loader.

Is it not enough to do this exactly for Javascript files?

I don't know if I understood the question correctly. You mean that instead of making a .map and replacing the existing JavaScript loader to include our loader what I should do is add another independent loader inside rules?

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 .js files, it only enters the necessary ones. With the idea that if these changes in the future because Next.js changes its internal structure, with this implementation our loader will not be broken.

But, now you have made me wonder that maybe this is not enough reason. So maybe it makes more sense to add an independent rule with our loader, making sure that it always runs before Babel. With this perhaps if it would be enough, excluding node_modules and little more.

@gurkerl83
Copy link
Contributor

gurkerl83 commented Nov 10, 2020

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

let use = [loader] // use has to be initialised with an empty array => let use = []

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.

webpack(conf, ...rest) {
      const config =
        typeof nextConfig.webpack === 'function'
          ? nextConfig.webpack(conf, ...rest)
          : conf

just replace it with

webpack(conf, options) {
      const config =
        typeof nextConfig.webpack === 'function'
          ? nextConfig.webpack(conf, options)
          : conf

Thx!

@aralroca
Copy link
Owner Author

@gurkerl83 thanks for this amazing feedback, I applied the changes under 1.0.0-experimental.4 😊 👍

@chrisss404
Copy link

chrisss404 commented Nov 20, 2020

Hi @aralroca, with 1.0.0-experimental.10 I get the following compile error when using the Trans component:

  Its return type 'string | ReactNode[]' is not a valid JSX element.
    Type 'string' is not assignable to type 'Element'.

  82 | 
  83 |             <Form.Text muted>
> 84 |                 <Trans i18nKey="signup:acceptTermsDescription" components={[<Link href="/terms">
     |                  ^
  85 |                     <a>terms</a>
  86 |                 </Link>]}/>
  87 |             </Form.Text>

Changing the return type to JSX.Element or React.ReactElement solves the compile error, however I don't know what is recommended in this case.

@aralroca
Copy link
Owner Author

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

@andrehsu
Copy link
Contributor

@aralroca
Using version1.0.0-experimental.13, I set up my next.config.js like this, but it doesn't work. Is this still how you apply the plugin?

const nextTranslate = require( 'next-translate')

module.exports = nextTranslate({})

@aralroca
Copy link
Owner Author

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

@andrehsu
Copy link
Contributor

@aralroca
I did not get that error, I was already on react 17.0.1.

My IDE inspection gave a warning on nextTranslate saying Method expression is not of Function type, don't know if that's relevant or not.

@aralroca
Copy link
Owner Author

@andrehsu
Copy link
Contributor

andrehsu commented Nov 23, 2020

@aralroca

Yes I am, but the issue exists independent of WebStorm.

@aralroca
Copy link
Owner Author

aralroca commented Nov 23, 2020

However, now I get this error:

Error was not caught ReferenceError: exports is not defined
    at Module.eval (webpack-internal:///./node_modules/next/dist/pages/_app.js:8)
    at eval (webpack-internal:///./node_modules/next/dist/pages/_app.js:108)

You can reproduce my setup here: https://github.com/jan-wilhelm/next-translate-error-reproduction

@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 App.getInitialProps() then TypeScript it compiles well. Fortunately, we have been able to dispense with this line of code and now with prerelease 1.0.0-experimental.15 it seems to work well with React 16.

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 🙏

@aralroca
Copy link
Owner Author

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

@BjoernRave
Copy link
Contributor

BjoernRave commented Nov 26, 2020

I tested 1.0.0-experimental.16". Things were running smooth, only switching language was not really working. This is my logic:

  const changeLanguage = (lng: string) => {
    if (router.asPath === '/') {
      router.push('/', '/', { locale: lng })
    } else {
      const routes = router.asPath.split('/')
      if (allLanguages.includes(routes[1])) {
        routes.splice(1, 1)
      }

      const route = routes.join('/') ? routes.join('/') : '/'

      router.push(route, route, { locale: lng })
    }
  }

the url path get's changed and the locale value on useRouer as well, but the translations stay the same.

my i18n.js:

module.exports = {
  locales: ['en', 'es', 'pt'],
  defaultLocale: 'en',
  loadLocaleFrom: (lang, ns) =>
    import(`./locales/${lang}/${ns}.json`).then((m) => m.default),
  pages: {
    '*': ['common'],
    '/': ['home'],
    '/signup': ['signup', 'forms'],
    '/socialsignup': ['signup', 'forms'],
    '/checkout': ['signup', 'forms'],
    '/privacy': ['privacy'],
    '/cookies': ['cookies'],
    '/terms': ['terms'],
    'rgx:/admin$': ['admin'],
    '/features': ['features'],
  },
}

@aralroca
Copy link
Owner Author

@BjoernRave I have copied your changeLanguage function in the examples of the experimental branch and tried several combinations and it seems to work in all cases. Perhaps it would help me if you could provide a reproducible example.

By the way, the last experimental one, for now, is the 1.0.0-experimental.18, with this one it happens too?

Could it happen on a particular page that you have getInitialProps on this page? There is this known issue, which has been going on since version 0.19:

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!

@BjoernRave
Copy link
Contributor

BjoernRave commented Nov 27, 2020

ah yes, getInitialProps might be the problem. When switching the language on a page without it, it works fine

@BjoernRave
Copy link
Contributor

so version 1.0 will not support the usage of getInitialProps ?

@aralroca
Copy link
Owner Author

aralroca commented Dec 1, 2020

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

  • getStaticProps
  • getServerSideProps
  • getInitialProps on _app.js

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.

@aralroca
Copy link
Owner Author

aralroca commented Dec 1, 2020

In fact, we have it implemented. If getInitialProps from Next.js sends the locale in its context, then it will work without another release.

@BjoernRave
Copy link
Contributor

got it, thanks for the clarification and your dedication to make everything work as smooth as possible :)

@aralroca
Copy link
Owner Author

aralroca commented Dec 1, 2020

Closing this. Already implemented on 1.0.0-canary.1. The 1.0 will be released approximately 1-2 weeks. 😊

@aralroca aralroca closed this as completed Dec 1, 2020
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

6 participants