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

Define functionality and features #1

Closed
isaachinman opened this issue Nov 9, 2018 · 31 comments
Closed

Define functionality and features #1

isaachinman opened this issue Nov 9, 2018 · 31 comments
Assignees

Comments

@isaachinman
Copy link
Contributor

isaachinman commented Nov 9, 2018

So we just completed a rewrite of the react-i18next NextJs example. This is/was an example, and we intentionally avoided abstracting or hiding things away, as we specifically wanted people to understand what was happening.

Through the process, several devs indicated that they would be interested in a next-i18next project which could be installed from npm and set up with minimal work.

This repo will potentially be that util, but we need to define a lot of characteristics before work can start.

What exactly are people looking for? How much of the configuration can be hidden away from users? How do we provide overrides?

At the very least:

  1. We will need to provide a HOC that users will wrap their _app.js component in.
  2. Users will need to import Link from us (not NextJs), if they would like to use language subpaths. There might be another solution here I'm not thinking of.
  3. We will need to provide some middleware that users will have to add to their server.js.
@Eruilz
Copy link

Eruilz commented Nov 9, 2018

My main issue with the example was routing. It is great, and I think necessary, to provide some basic implementation of subpath.
But by doing so the routing logic became split in 3 or 4 places : server.js, _app.js (on language change), your Link component, and eventually something for declarative navigation (router.push(...,...)).

  • it makes it rather "hard" (but not really !) to implement conversion from mypage?id=value to mypage/value as explained in the very first tutorial everybody does on nextjs (https://nextjs.org/learn/basics/clean-urls-with-route-masking)
  • I think that your example struggle quite a bit with querystring (but maybe it was something I did)

I implemented something that fill my needs nicely (with a config file for route and some "helper" reading it, so to have it all in one place), but I guess some pointer in comments might be usefull ( like //route masking is managed here !)

@isaachinman
Copy link
Contributor Author

Hi @Eruilz. So, in this project, you would import several things from next-i18next:

server.js

import { nexti18NextMiddleware } from 'next-i18next'

const app = express()
app.use(nexti18NextMiddleware({ localeSubpaths: true }))

component.js

import { Link } from 'next-i18next'
...
<Link href="/url-ignoring-language">

Or at least, this is my first thoughts/approach. Does this seem simple enough? It is true that imperative use of router.push could be tricky.

It may be possible to rewrite the subpath functionality as a next/router middleware using router.replace, but we had some trouble with that in the first iteration of the example.

In general I would prefer to hook into routeChangeStart, but I am not sure this is a viable approach.

What do you think?

@Eruilz
Copy link

Eruilz commented Nov 10, 2018

@isaachinman well routeChangeStart looks like a good idea... until you look into nextjs code where it is said to be deprecated "soonish"... so not really sure that should be the way :)

In the example, it was "simple" to use your implementation, but confusing (at least that is what I thought) to add route masking functionality.
Maybe you should look into https://github.com/fridays/next-routes

@isaachinman
Copy link
Contributor Author

@Eruilz I did not see that it was being deprecated soon. Can you link to that? I'm not actually convinced that it's better to intercept next/router events, except in that it might be "easier" for the user.

We're well aware of next-routes and have chosen to avoid it. @timneutkens warns against it in many places, most prominently here.

Any comments about that potential installation / use flow above?

@timneutkens
Copy link

routeChangeStart is far from deprecated, we changed the API a bit, it's now an EventEmitter (previously this was hidden to users) as it allows multiple listeners vs just 1 that has to be managed by the user.

@revskill10
Copy link
Contributor

revskill10 commented Nov 10, 2018

  • One more thing i want to see is the new V2 on Now with lambda and builders.
    Currently we have to use Express Middleware, which i see unsuitable for V2 deployment type. So we might need two subpackages /v1 and /v2.

  • About route event, i prefer not to use any of imperative methods outside of a React component. Because nextjs provides us hoc to inject router into React component (withRouter)

@Eruilz
Copy link

Eruilz commented Nov 12, 2018

I must have misunderstood something then, sorry :)
Here is where I found that deprecated thing : https://github.com/DefinitelyTyped/DefinitelyTyped/blob/7e0a27458d3a333357002e7f0d1f09337eb2af4d/types/next-server/router.d.ts#L60

@isaachinman
Copy link
Contributor Author

@timneutkens Thanks for chiming in and letting me know - that's good to hear. Out of curiosity, do you think hooking into routeChangeStart to perform route replacements to enable paths as shown below is an appropriate implementation, or should we stick to the wrapped Link component here?

/about
/fr/about
/de/about

@revskill10 Do you have an example of a Lambda-friendly NextJs project with custom routes? As far as I know, it will be literally impossible to support custom routes with NextJs without manually declaring our Express server, etc. Could be totally wrong there.

@revskill10 Also, can you explain what you mean about imperative methods? Normal use case would involve importing Link from either next/router or next-i18next, depending on how we proceed here. I think @Eruilz point was that eventually some users will need to perform route changes imperatively (ie inside a Redux action, and so on), and will get themselves into trouble if they push/replace to a route in the simplistic way without using as.

@timneutkens
Copy link

or should we stick to the wrapped Link component here?
Definitely this 👍

@isaachinman
Copy link
Contributor Author

@timneutkens Do you mind explaining why? I'm happy to proceed that way, but it does create a "less simple" solution for the end user, and can result in some gotchas when it comes to imperative use of the router.

@Eruilz
Copy link

Eruilz commented Nov 12, 2018

@isaachinman what about using some helper calling next/router functions ? so we would load

import { Router} from 'next-i18next'
 ...
router.push("/url-ignoring-language", optional as, {option})
router.replace("/url-ignoring-language", optional as, {option})

Would that be acceptable or are we running in the same scalability issues ?

@isaachinman
Copy link
Contributor Author

Hey @Eruilz, that's a great idea. We can export both Link and Router from the next-i18next package. That should allow users to do whatever they'd like to, essentially.

I think we're actually pretty close to being able to start working on a pre release.

My last concern is: the config will be exactly the same as this, but we'll need to think of a good way to allow users to override settings, both for the client and server.

Our i18n instance will be handled completely internally and will not be exposed to the end user at all. I suppose instead of doing if (!process.browser), we can create a nested ssr object within the config so that users can pass everything at once.

@Eruilz
Copy link

Eruilz commented Nov 12, 2018

I use i18n instance to retrieve current language and display country flag according to current language, there could be other use case I guess.
Maybe let some basic properties readable ?
I can pass something in a translation file aswell, but that seems less intuitive (works great nonetheless).

/locales/en/lang.json : 
{
  lng:"en"
}

I may also have missed something and there is an other way ?

@isaachinman
Copy link
Contributor Author

@Eruilz Thanks for pointing that out. Perhaps it's just best to export the i18n instance after all, and make it clear via documentation that users should only import/change the instance if they understand what it does.

I forgot to include in the server/component example above, that users will also need to take a third step: create an _app.js file, and do:

_app.js:

import React from 'react'
import App, { Container } from 'next/app'
import { appWithTranslation } from 'next-i18next'

@appWithTranslation
export default class MyApp extends App {
  static async getInitialProps({ Component, router, ctx }) {
    let pageProps = {}

    if (Component.getInitialProps) {
      pageProps = await Component.getInitialProps(ctx)
    }

    return { pageProps }
  }

  render () {
    const { Component, pageProps } = this.props

    return (
      <Container>
        <Component {...pageProps} />
      </Container>
    )
  }
}

I'm still not clear on exactly how a HOC on _app.js will work in regards to getInitialProps. Will need to figure that out when the time comes.

@timneutkens
Copy link

I wonder if we could introduce some kind of extensibility point for this, because exposing the router through the i18n package would mean that you'd be responsible for API-compatibility and it would confuse new users that read the Next.js documentation and then use next/router instead.

@timneutkens
Copy link

I just realized the i18n link might not cover all next/link use cases:

https://github.com/i18next/react-i18next/blob/master/example/nextjs/components/Link.js#L32

For example you could do:

<Link href="/about?something=123">

Which would end up being

<Link href="/about?something=123?lng=en">

Also next/link accepts objects that are ran through url.format()

@timneutkens
Copy link

I wonder if we could introduce some kind of extensibility point for this, because exposing the router through the i18n package would mean that you'd be responsible for API-compatibility and it would confuse new users that read the Next.js documentation and then use next/router instead.

To come back to this, I think it makes a lot of sense to actually not expose the global Router but instead use React's component model, meaning wrap withRouter and expose it as withI18nRouter with the push/replace methods implemented.

@isaachinman
Copy link
Contributor Author

isaachinman commented Nov 13, 2018

Hey @timneutkens.

exposing the router through the i18n package would mean that you'd be responsible for API-compatibility

I think we'd just be importing directly from next/router and then simply doing some pre-processing on a select few methods - probably just replace and push. Any changes to the replace or push methods would require action by us, but everything else should be passed on without issue. Basically, we're going to have to take responsibility for a small portion of the Router API no matter which way the functionality is written. Unless I'm misunderstanding.

it would confuse new users that read the Next.js documentation and then use next/router instead

Yes, that is the main concern. In general it's going to be very tricky to make a localised SSR app "easy", and I'd rather not build in confusing patterns.

I just realized the i18n link might not cover all next/link use cases:

Yes, that's correct. My goal with that example was to keep things as simple and easy-to-understand as possible - it's a starting point for devs. If we're proceeding here with a next-i18next, not an example, that would of course need refactoring to cover all use cases (including params as you pointed out).

wrap withRouter and expose it as withI18nRouter with the push/replace methods implemented

This is an interesting suggestion. I do not have previous experience using withRouter - in past NextJs projects I've always imported Router directly. Going for something like withI18nRouter might make more sense for maintainers, but it wouldn't do much to reduce confusion with people importing Router from next/router and expecting it to work. Perhaps that's just something we'll have to live with, and will end up closing a lot of "bug" reports.

@isaachinman isaachinman self-assigned this Nov 13, 2018
@isaachinman isaachinman added this to the Initial Release milestone Nov 13, 2018
@isaachinman
Copy link
Contributor Author

isaachinman commented Nov 13, 2018

@timneutkens As far as extensibility, I've always felt as though there should be another Router event, called validateRouteChange or something similar (naming is hard), where you could return either:

  1. true - routing procedure continues as normal
  2. false - routing procedure is cancelled
  3. A route string - redirect happens before routing procedure starts, negating the need for Router.replace.

This would allow the kind of functionality we're talking about here, but also allow for protecting logged-in routes, and so on. Just a thought, it could be ill-advised for some reason unbeknownst to me.

Update: it's already been discussed here and here, and probably elsewhere. No point discussing over in this repo.

@timneutkens
Copy link

We don't want to add global routing side-effects to the router.

I think we'd just be importing directly from next/router

This is actually something I've been thinking about deprecating for a long time, it's a stateful module, which makes code-splitting it harder whereas withRouter fits itself into the React component model.

@isaachinman
Copy link
Contributor Author

thinking about deprecating for a long time

Probably a good idea. It might also make a lot of sense to line that up with the new React hooks stuff.

For this next-i18next project, I'll plan to go forward with a wrapped withI18nRouter as you suggested. Thank you very much for the feedback.

@timneutkens
Copy link

Yeah, using the router will look like this: https://twitter.com/timneutkens/status/1057053195044823040

@isaachinman
Copy link
Contributor Author

@revskill10 Any update about Lambda support before I get started on this?

@revskill10
Copy link
Contributor

@isaachinman What i think is we should limit configuration on server side, to adapt i18next for lambda function.
I love the simplicity of function(req, res) . No middleware, no express, so that it could be used every where, no matter what the framework is used.
On client side, i agree with what @timneutkens said above.

@isaachinman
Copy link
Contributor Author

@revskill10 I do not understand what you mean at all. This project sets out to be a drop-in utility to get react-i18next quickly working in NextJs projects.

If you want changes made to the i18next core, you should request them there.

This project will definitely be exporting a middleware, which is indeed just a function that takes req, res, next... that's what a middleware is. It will also definitely use i18next-express-middleware and i18next-node-fs-backend.

If you don't have access to a filesystem or Express/Koa/http, I honestly don't know how you plan to achieve SSR with localisation. Feel free to open an issue on whatever repo you think is appropriate for your feature request.

@DmacLoyaltyOne
Copy link

With this example, is there any way to use SSR with hooks on the frontend?

@isaachinman
Copy link
Contributor Author

@DmacLoyaltyOne Can you explain your use case? If you're referring to React hooks, that functionality has not even landed in React core yet, and is of course not supported here until that happens.

@DmacLoyaltyOne
Copy link

@isaachinman Yes that is what I was referring to. Since there is a section devoted to using react-i18next with hooks in the documentation, I figured there might be a way to get it to work with SSR, but you're telling me that is not the case yet, correct?

@isaachinman
Copy link
Contributor Author

isaachinman commented Nov 20, 2018

@DmacLoyaltyOne I think what you're referring to is @jamuhl's work in preparation for v10. This repository is a WIP, and will be a tool to quickly and easily add i18next to NextJs projects. Support in this project for hooks depends on: (1) React release, (2) react-i18next v10 release. So it's awhile down the roadmap. If you're interested in using hooks ASAP, I'd suggest you take a look at our example in the react-i18next repo, and roll it yourself.

I have not yet played with hooks in relation to SSR, but I don't see how it'd make any difference vs clientside.

If you want to continue discussion on this topic, please open an issue in react-i18next.

@isaachinman
Copy link
Contributor Author

Hello everyone - I will be releasing v0.1.0 of this project on npm shortly. I would appreciate some early testers to provide feedback if anyone can do that. Thanks!

@Eruilz
Copy link

Eruilz commented Nov 25, 2018

Thx @isaachinman , I will take a look ASAP (right now, it's family time !)

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

5 participants