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

Basename and dynamic basename #72

Open
klis87 opened this issue Jan 3, 2020 · 7 comments
Open

Basename and dynamic basename #72

klis87 opened this issue Jan 3, 2020 · 7 comments

Comments

@klis87
Copy link

klis87 commented Jan 3, 2020

All my urls start with lang, like /en, /en/contact etc. A perfect case for basename, especially dynamic one. I considered to switch from RFR to Rudy, because based on https://github.com/respond-framework/rudy/blob/master/packages/rudy/src/actions/changeBasename.js I figured that in Rudy we can change basename, so lang change would be much easier. Sadly, it doesn't work I am afraid.

I have:

  const {
    reducer: routerReducer,
    middleware: routerMiddleware,
    firstRoute,
    api,
  } = createRouter(routesMap, {
    basename: '/en',
  });

but basename does nothing, generally I migrated the whole app to Rudy and it seems to work fine, but basename is '' looking at Redux location state.

If this is not supported, could you consider adding this dynamic basename feature to RFR? This is very common use case for routers, and right now I need to have :lang param in all urls which is very inconvenient.

@hedgepigdaniel
Copy link
Collaborator

The dynamic basename should work in Rudy, although I haven't tried it myself.

If you're willing to do a bit of digging to work out why it doesn't work I'd be interested to hear why. Perhaps there is a simple bug somewhere (e.g. the basename does not get passed correctly to the path matching functions)?

@hedgepigdaniel
Copy link
Collaborator

Looking through the code, it looks like the option is basenames: string[], not basename: string. There is a static list of possible basenames, and you can switch between them dynamically.

Does that fix the problem?

@klis87
Copy link
Author

klis87 commented Jan 4, 2020

basenames does seems to work, thx!

@klis87 klis87 closed this as completed Jan 4, 2020
@klis87
Copy link
Author

klis87 commented Jan 4, 2020

There is one issue though, with basename like /en and path like / I am getting /en/ path. I dont want leading slashes though. The problem is that path: '' doesnt work. For path: '/contact' url is fine though - /en/contact, this is inconsistent though, is this possible not to have leading slash after basename for homepage?

@klis87 klis87 reopened this Jan 4, 2020
@hedgepigdaniel
Copy link
Collaborator

You could try having the empty string as one of the basenames.

Failing that, I would say that this is correct behaviour - the basename is something that is prepended to ALL routes. If you want only some routes to have it, perhaps you could make a param in each of the routes where you want it, e.g. /:lang/...

@klis87
Copy link
Author

klis87 commented Jan 6, 2020

Actually I want basename for all routes, including homepage, just the issue is that with basename: '/en' and route home path: '/' the url becomes /en/ while I want to have /en

@hedgepigdaniel
Copy link
Collaborator

Hmm, that makes sense. Two things to keep in mind:

  • the empty string is not a valid path in HTTP (although trailing slashes are not required otherwise). The path of the root is always / and not the empty string, even if you sometimes see that in the UI.
  • the route / contains a trailing slash, so it is expected that it appears in the URL compiled by Rudy (not only / but also /en/)

It sounds to me like what you want is the ability to have the empty string as a route path, so that its possible to have a route path that does not add any segments, and also does not require a trailing slash. In the code, the exception for the root route (which cannot be the empty string) can be moved deeper so that it does not affect paths that include a basename and therefore don't require a trailing slash.

I would welcome a patch to do this. There are tests in the packages/rudy package that exercise actionToUrl and urlToAction. I'd suggest starting there, and grepping the code for '/' and "/".

My personal opinion: Always use trailing slashes in your URLs. If there is no trailing slash in a request, redirect to a URL that does have one. That way you don't run into these issues because the root path is not an exception to the rule.

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

2 participants