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

refactor: Use rewrites instead of custom server #689

Merged
merged 1 commit into from
May 1, 2020
Merged

Conversation

isaachinman
Copy link
Contributor

This is a first start at refactoring out usage of a custom server.

This relies upon the incoming rewrites NextJs feature, which is still in an experimental phase. By combining the rewrites array with middleware inside _app's getInitialProps, we can achieve the same result as what previously required a custom server.

Note that until NextJs supports middleware for normal routes (not just API routes), we'll need to take this getInitialProps middleware approach.

For rewrites to work, localeSubpaths need to be available both at build time (for NextJs to implement rewrites), and at run time (for next-i18next to implement 302s, etc). To make this as easy as possible, I've dropped localeSubpaths into publicRuntimeConfig in the example.

The future of next-i18next will involve periodic and signficant rewrites as NextJs slowly releases the features which we need to actually support serverless targets in a first class way. Notable examples are normal-route-middlewares and plugin support. This work, however, is a good first start to these incremental refactors.

Links

vercel/next.js#9081
vercel/next.js#9133
vercel/next.js#7208

@martpie
Copy link

martpie commented Apr 21, 2020

What could be really useful for me would be to have an example of updated README.md to know exactly how the setup changes and if there are any trade-offs :)

@isaachinman
Copy link
Contributor Author

@martpie Sure, I've updated the README now. However, I'm mostly hoping for actual code reviews, as I'm not sure if I've missed any edge cases, especially in the implementation having to do with rewrites and passing the lang as a query param.

Here's how the setup varies from the current next-i18next flow:

  1. No need for a custom server whatsoever
  2. If you want locale subpaths, you need to pass them in via your next.config.js as well

That's it. There shouldn't be any trade-offs, unless I've missed something!

@martpie
Copy link

martpie commented Apr 22, 2020

It looks really great, I'll try to go through the changes.

Is there any plan to support getServerSideProps? (or getStaticProps)

@isaachinman
Copy link
Contributor Author

@martpie The main drawback at the moment is that rewrites is still experimental on the NextJs side, so we can't really consider this production ready in any sort of way.

As far as the other data fetching methods: at first glance it seems like getServerSideProps is the perfect fit, but _app cannot have getServerSideProps (vercel/next.js#10874).

And getStaticProps would never work, as we need to initialise an i18n instance based on the request language.

Keep in mind that next-i18next will change dramatically when first-class plugin support lands.

@isaachinman
Copy link
Contributor Author

isaachinman commented Apr 22, 2020

Ah: something I'm not currently sure of, is if importing the middleware in app-with-translation is causing that code to be pulled into the client bundle. That would be something to investigate. We may have to do more env-specific dynamic imports.

@isaachinman
Copy link
Contributor Author

Another thing to check is if this rewrites approach is even correct. We need to whitelist the value of the locale subpath, but pass the key into query.lang.

@gurkerl83
Copy link

Importing the middleware in the app-with-translation hoc might cause the loading issue you described. With my experiment, which I did some time ago, I have integrated a custom middleware in _document, and the bundle sizes just exploded. Also, google speed insights tells that a large portion of the loading time gets spent interpreting code, but there is not much there. Removing the hoc entirely results in bundle sizes of a few dozen kb max, so I guess everything was pulled to the client. Another thing that made things for me very complicated to understand is the app-with-translation hoc and its internals. In the context of SSR, which is already quite challenging, things like hoisting, possible side effects with the other libraries included made things very complex to go forward. I have not looked at the project next-translate, but what they are proposing is that _app.js, _document.js, and _error.js are not going to be wrapped with the translations context, so it's not possible to translate these files directly. Maybe this is worth some exploring.

Insane-Size

@isaachinman
Copy link
Contributor Author

@gurkerl83 Not sure if the NextJs tools are detailed enough to be trusted on this. Something like webpack-bundle-analyzer is probably better.

If the NextJs tools are to be trusted, here are the two outputs:

With Custom Server (existing implementation)

with_custom_server

With Rewrites (new implementation)

with_rewrites

Feel free to give it a try yourself. It looks like there's no difference, however I'm hesitant to take that at face value.

@Vadorequest

This comment has been minimized.

@isaachinman isaachinman changed the base branch from master to beta May 1, 2020 16:40
@isaachinman isaachinman merged commit 529486b into beta May 1, 2020
@isaachinman isaachinman deleted the use-rewrites branch May 1, 2020 16:44
isaachinman added a commit that referenced this pull request Jul 27, 2020
* refactor: Use rewrites instead of custom server (#689)

* chore: Add .e2e to npmignore

* v5.0.0-beta.0

* chore: Add scripts to npmignore

* v5.0.0-beta.1

* 5.0.0-beta.2

* refactor: Require absolute localePath, and refactor out usage of eval

* 5.0.0-beta.3

* refactor: Clean up example dir, remove class components

* chore: Update core-js to v3

* docs: Update README

* v5.0.0-beta.4

* Clean up release
@santi020k
Copy link

How work custom routes with rewrites ?

I'm working with diferent paths

Like:

  • example.com/en/test
  • example.com/es/prueba

@santi020k
Copy link

I solver with

  rewrites: async () => [
    ...nextI18NextRewrites(localeSubpaths),
    // EN Routers
    ...englishPaths.map(({ source, destination }) => ({
      source,
      destination,
    })),

    // ES Routers
    ...spanishPaths.map(({ source, destination }) => ({
      source,
      destination,
    })),
  ],

In case someone is helpful

@isaachinman
Copy link
Contributor Author

@SantiagoMolinaOrozco What are englishPaths and spanishPaths? I think that is indeed very helpful, and we should add it to the documentation.

@santi020k
Copy link

santi020k commented Jul 30, 2020

@isaachinman sure, I'm going to share the full example:

routes.js

const spanishPaths = [
  {
    source: "/terminos-y-condiciones",
    destination: "/terms",
  },
  {
    source: "/brokers/:slug",
    destination: "/brokers/:slug",
  },
  {
    source: "/brokers",
    destination: "/brokers",
  },
];

const englishPaths = [
  {
    source: "/terms-and-conditions",
    destination: "/terms",
  },
  {
    source: "/brokers/:slug",
    destination: "/brokers/:slug",
  },
  {
    source: "/brokers",
    destination: "/brokers",
  },
];

module.exports = { spanishPaths, englishPaths };

next.config.js

const { spanishPaths, englishPaths } = require("./routes");
const localeSubpaths = { es: "es", en: "en" };

const nextConfig = {
  webpack: (config) => {
    config.node = {
      fs: "empty",
    };
    return config;
  },
  publicRuntimeConfig: { localeSubpaths },
  rewrites: async () => [
    ...nextI18NextRewrites(localeSubpaths),
    // EN Routers
    ...englishPaths.map(({ source, destination }) => ({ source, destination })),

    // ES Routers
    ...spanishPaths.map(({ source, destination }) => ({ source, destination })),
  ],
};

somthing like that, i try to summarize my current configuration, improvement recommendations are received.

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

Successfully merging this pull request may close these issues.

5 participants