vikejs / vike

🔨 Flexible, lean, community-driven, dependable, fast Vite-based frontend framework.
https://vike.dev
MIT License
4.25k stars 349 forks source link

routes ending with `/` are not handled #949

Closed NilsJacobsen closed 1 year ago

NilsJacobsen commented 1 year ago

Description

image

Routes that end with / are not handled to redirect to the prior route.

Expected

domain.com/docs/ -> domain.com/docs

Is that something that you want to integrate by default? I'm sure 99% of the cases this is gonna be wanted.

brillout commented 1 year ago

Does it also happen in production?

NilsJacobsen commented 1 year ago

Jap also in production.

image

brillout commented 1 year ago

Can you configure your production server to do the redirection?

I agree it's something that should be built into VPS. I updated https://github.com/brillout/vite-plugin-ssr/issues/926.

NilsJacobsen commented 1 year ago

Thanks, we can handle it until it is build in

brillout commented 1 year ago

Done, and will be released in 0.4.134.

martijndirksen commented 1 year ago

It appears I'm the unfortunate 1% here that actually wants the trailing slash here. Is it possible to make the normalization behavior optional? For example a flag in the vps configuration:

// vite.config.ts
ssr({
  normalizePathnames: false,
}),
brillout commented 1 year ago

@martijndirksen @martijnpor I was expecting that someone may need to disable this. Why do you want the trailing slash?

martijndirksen commented 1 year ago

We migrated a production platform to vite-plugin-ssr which was already using trailing slashes in its routing. We're left with permanent redirects and we cannot easily go back to a situation without them.

Regardless of our own situation, I feel like enforcing URL conventions is not necessary for vite-plugin-ssr. The users can figure out appropriate solutions. As it stands now it might be too restrictive for framework consumers.

brillout commented 1 year ago

:warning: DO NOT use the workaround without writing a comment explaining your use case.

Workaround Remove the trailing slash in renderPage(): ```js import { renderPage } from 'vite-plugin-ssr/server' server.addMiddleware({ method: 'GET', route: '*', // Catch-all async handler(request) { let { url } = request // Removing trailing slash if (url.endsWith('/') && url !== '/') url = url.slice(0, -1) const pageContextInit = { urlOriginal: url } const pageContext = await renderPage(pageContextInit) // ... } }) ```

If there is a (not-too-niche) use case for it, then there should be a new config for how vite-plugin-ssr should treat trailing slashes.

martijndirksen commented 1 year ago

With a slight adjustment, your proposal seems to work. We still use trailing slash redirects (which we do ourselves) and then fool vps by removing that trailing slash when passing in the urlOriginal. Note that we have to check for / as we can't pass an empty string to the urlOriginal.

This workaround is a bit nicer than me using patch-package to strip out the url normalization from your package, but it still feels unintuitive. I need to place a comment in my source code to explain why we strip the trailing slash again, to avoid developers from removing this and getting redirect loops.

const pageContextInit: PageContextInit = {
  urlOriginal: req.originalUrl !== '/' && req.originalUrl.endsWith('/') ? req.originalUrl.slice(0, -1) : req.originalUrl,
  // ...
};

Our general use case is that our clients have different preferences when it comes to the usage of trailing slashes. Some believe there is a difference in SEO, but there does not seem to be a clear consensus around this. We therefore need to be able to support both with and without. Automatic URL normalization by vite-plugin-ssr would make things more difficult for us in this regard. If you want that kind of stuff from a framework, wouldn't something like Nuxt be a better fit?

brillout commented 1 year ago

A new config isn't worth it, if it's used only by a handful of users while there is valid workaround. Duct taping is obviously not ideal, but too many configs isn't either. That's why I'm inclined to create a new config only if I can see a use case for it.

We therefore need to be able to support both with and without

What's the benefit of disabling trailing slash redirection?

We still use trailing slash redirects (which we do ourselves)

Why is vite-plugin-ssr's trailing slash redirection causing a problem then?

schaschko commented 1 year ago

@martijndirksen same approach here ;)

I'm addressing possible issues related to duplicate content, therefore implementing a 301 from non-trailing to trailing slash location.

brillout commented 1 year ago

@schaschko If you don't mind elaborating why you need trailing slashes, I'll further consider adding a config for how vite-plugin-ssr should treat trailing slashes.

I can't think of a website I use that has trailing slashes, so I'm inclined to think that your use case is very uncommon.

schaschko commented 1 year ago

@brillout Yes of course, please check regarding trailing slashes. It's a decision whether to serve with or without a trailing slash. Imo it should not be enforced by vps (e.g. you should not be forced to work around, but encouraged to configure).

And I don't think the use case is very uncommon ;)

brillout commented 1 year ago

Quickly glazed over your link, but I still don't see the benefit of trailing slashes?

To be clear, I'm very much open for a new config trailingSlash: true | false | null (null disables the auto redirect).

And I don't think the use case is very uncommon ;)

In that case someone will quickly be able to explain me why trailing slashes can be usefull and I'll implement the config then ;)

schaschko commented 1 year ago

I know what you mean bro, but vps should not bypass the dev imo.

https://example.com/notthesame could serve completely different content than https://example.com/notthesame/ (it won't). As different as https://example.com/path/to/some/thing vs. https://example.com/path/to/some/other/thing, but we wouldn't automatically "normalize" /path/to/some/thing to resolve to /path/to/some/other/thing. I know, not the same thing semantically.

In the end, I think the dev should be in control on how to handle urls pointing to different resources, useful or not.

MichaelRoosz commented 1 year ago

yes trailing slashes are mostly a cosmetic thing. but if you are already are using them, then why remove them and add redirects to all your pages? this will impact SEO (redirecting all pages). also some people just prefer to have them.

in my case, having trailing slashes is a hard requirement, not decided by myself, so having such an option would be very helpful. for now I am also messing with "urlOriginal"

brillout commented 1 year ago

I'll implement a new config for it.

brillout commented 1 year ago

https://vite-plugin-ssr.com/url-normalization released in 0.4.140.

brillout commented 1 year ago

Thanks for the conversation.

We're looking for sponsors, in particular company sponsorships: https://github.com/sponsors/brillout.