vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.73k stars 26.85k forks source link

Nextjs should have more options for trailing slash. #15148

Closed armspkt closed 3 years ago

armspkt commented 4 years ago

Feature request

Is your feature request related to a problem? Please describe.

Nuxtjs have several options to choose trailing slash. (undefined, true, false) nuxt-community/nuxt-i18n#422

Describe the solution you'd like

Nextjs should have more options for trailing slash. If use undefined Nextjs should not redirect route when use URL that ends with a slash

trailingSlashes: true

/abc/ -> /abc/ (keeps the same) /abc -> /abc/ (changes)

trailingSlashes: false

/abc -> /abc (keeps the same) /abc/ -> /abc (changes)

trailingSlashes: undefined (default)

/abc/ -> /abc/ (keeps the same) /abc -> /abc (keeps the same)

Janpot commented 4 years ago

@armspkt Can you expand a bit more on why you need this behavior (other than "just because nuxt does it this way"). Is there a specific use-case you're trying to solve? What's the practical use of having different urls point to the same content? Is there a benefit to having that behavior?

armspkt commented 4 years ago

@Janpot I need this feature because many users on my website access via both (with a slash and without a slash) and I think it good to have those options that we don't have to redirect for the better user experience and I saw many sites work that way too.

There is no specific use-case for Nextjs yet but There is a case about i18n localePath for Nuxtjs. https://github.com/nuxt/nuxt.js/pull/6331

I think it better to have more options to choose from.

Is there a specific use-case you're trying to solve?

What's the practical use of having different urls point to the same content?

https://www.youtube.com/feed/trending and https://www.youtube.com/feed/trending/ https://github.com/pulls and https://github.com/pulls/ https://twitter.com/explore and https://twitter.com/explore/ https://www.facebook.com/groups/feed and https://www.facebook.com/groups/feed/

Can you explain why those websites did not redirect?

Is there a benefit to having that behavior?

I don't want it because Nuxtjs have it but I want it because it is nice to have feature. That's all.

nikosantis commented 4 years ago

Yeah, i think we need that, idk if this solutions is the best, but, need a solution. How nextjs do this https://nextjs.org/showcase and https://nextjs.org/showcase/?

sebqq commented 4 years ago

Yeah, i think we need that, idk if this solutions is the best, but, need a solution.

How nextjs do this https://nextjs.org/showcase and https://nextjs.org/showcase/?

Check their website's next.config.js file. I think that you will find your answer there.

nikosantis commented 4 years ago

Yeah, i think we need that, idk if this solutions is the best, but, need a solution. How nextjs do this https://nextjs.org/showcase and https://nextjs.org/showcase/?

Check their website's next.config.js file. I think that you will find your answer there.

Now in "next": "^9.4.5-canary.32" have a solution by default :)

Unsfer commented 4 years ago

Something wrong going on when using basePath and trailingSlash: true: Next/Link removes trailing slash when going to "/" And in some old browsers got error (which was not in 9.4.5-canary.27)

Unhandled promise rejection,TypeError: t.entries is not a function
/_next/static/runtime/polyfills-35e39b6a0bf08dcb376c.js

Using 9.4.5-canary.34

Janpot commented 4 years ago

@Unsfer Do you know, by any chance, which browser throws this error?

Unsfer commented 4 years ago

It was some old version of FF, which doesn't sopport Object.entries. I think it's supposed to be polyfilled

aralroca commented 4 years ago

A richer system would be very good to be able to decide in which paths redirects should be avoided at all costs. For example, in our case, as I commented here:

https://github.com/vercel/next.js/issues/15867#issuecomment-683748417

We want to avoid 308 before 404 pages.

We want this:

old_behavior

And the current feature forces to:

trailingslash

timneutkens commented 4 years ago

@aralroca feel free to reach out to enterprise@vercel.com and we'll see what we can do for your specific case under enterprise support.

u3u commented 4 years ago

By default Next.js will redirect urls with trailing slashes to their counterpart without a trailing slash.

Can I disable it? Or can it be used only as a designated route? I think this is a good feature, but my boss requires the URL to remain the same. 😥 If I use patch-package to remove the default redirection and removePathTrailingSlash, will there be any side effects?

diff --git a/node_modules/next/dist/client/normalize-trailing-slash.js b/node_modules/next/dist/client/normalize-trailing-slash.js
index 9ceda38..71b277b 100644
--- a/node_modules/next/dist/client/normalize-trailing-slash.js
+++ b/node_modules/next/dist/client/normalize-trailing-slash.js
@@ -1,6 +1,6 @@
 "use strict";exports.__esModule=true;exports.removePathTrailingSlash=removePathTrailingSlash;exports.normalizePathTrailingSlash=void 0;/**
  * Removes the trailing slash of a path if there is one. Preserves the root path `/`.
- */function removePathTrailingSlash(path){return path.endsWith('/')&&path!=='/'?path.slice(0,-1):path;}/**
+ */function removePathTrailingSlash(path){return path;}/**
  * Normalizes the trailing slash of a path according to the `trailingSlash` option
  * in `next.config.js`.
  */const normalizePathTrailingSlash=process.env.__NEXT_TRAILING_SLASH?path=>{if(/\.[^/]+\/?$/.test(path)){return removePathTrailingSlash(path);}else if(path.endsWith('/')){return path;}else{return path+'/';}}:removePathTrailingSlash;exports.normalizePathTrailingSlash=normalizePathTrailingSlash;
diff --git a/node_modules/next/dist/lib/load-custom-routes.js b/node_modules/next/dist/lib/load-custom-routes.js
index c56f7e4..f67024f 100644
--- a/node_modules/next/dist/lib/load-custom-routes.js
+++ b/node_modules/next/dist/lib/load-custom-routes.js
@@ -5,5 +5,5 @@ const errMatches=err.message.match(/at (\d{0,})/);if(errMatches){const position=
 // for not being a string
 const{tokens,error}=tryParsePath(route.source);if(error){invalidParts.push('`source` parse failed');}sourceTokens=tokens;}// make sure no unnamed patterns are attempted to be used in the
 // destination as this can cause confusion and is not allowed
-if(typeof route.destination==='string'){if(route.destination.startsWith('/')&&Array.isArray(sourceTokens)){const unnamedInDest=new Set();for(const token of sourceTokens){if(typeof token==='object'&&typeof token.name==='number'){const unnamedIndex=new RegExp(`:${token.name}(?!\\d)`);if(route.destination.match(unnamedIndex)){unnamedInDest.add(`:${token.name}`);}}}if(unnamedInDest.size>0){invalidParts.push(`\`destination\` has unnamed params ${[...unnamedInDest].join(', ')}`);}else{const{tokens:destTokens,error:destinationParseFailed}=tryParsePath(route.destination,true);if(destinationParseFailed){invalidParts.push('`destination` parse failed');}else{const sourceSegments=new Set(sourceTokens.map(item=>typeof item==='object'&&item.name).filter(Boolean));const invalidDestSegments=new Set();for(const token of destTokens){if(typeof token==='object'&&!sourceSegments.has(token.name)){invalidDestSegments.add(token.name);}}if(invalidDestSegments.size){invalidParts.push(`\`destination\` has segments not in \`source\` (${[...invalidDestSegments].join(', ')})`);}}}}}const hasInvalidKeys=invalidKeys.length>0;const hasInvalidParts=invalidParts.length>0;if(hasInvalidKeys||hasInvalidParts){console.error(`${invalidParts.join(', ')}${invalidKeys.length?(hasInvalidParts?',':'')+` invalid field${invalidKeys.length===1?'':'s'}: `+invalidKeys.join(','):''} for route ${JSON.stringify(route)}`);numInvalidRoutes++;}}if(numInvalidRoutes>0){if(hadInvalidStatus){console.error(`\nValid redirect statusCode values are ${[...allowedStatusCodes].join(', ')}`);}console.error();throw new Error(`Invalid ${type}${numInvalidRoutes===1?'':'s'} found`);}}async function loadRedirects(config){if(typeof config.redirects!=='function'){return[];}const _redirects=await config.redirects();checkCustomRoutes(_redirects,'redirect');return _redirects;}async function loadRewrites(config){if(typeof config.rewrites!=='function'){return[];}const _rewrites=await config.rewrites();checkCustomRoutes(_rewrites,'rewrite');return _rewrites;}async function loadHeaders(config){if(typeof config.headers!=='function'){return[];}const _headers=await config.headers();checkCustomRoutes(_headers,'header');return _headers;}async function loadCustomRoutes(config){const[headers,rewrites,redirects]=await Promise.all([loadHeaders(config),loadRewrites(config),loadRedirects(config)]);if(config.trailingSlash){redirects.unshift({source:'/:file((?:[^/]+/)*[^/]+\\.\\w+)/',destination:'/:file',permanent:true},{source:'/:notfile((?:[^/]+/)*[^/\\.]+)',destination:'/:notfile/',permanent:true});if(config.basePath){redirects.unshift({source:config.basePath,destination:config.basePath+'/',permanent:true,basePath:false});}}else{redirects.unshift({source:'/:path+/',destination:'/:path+',permanent:true});if(config.basePath){redirects.unshift({source:config.basePath+'/',destination:config.basePath,permanent:true,basePath:false});}}return{headers,rewrites,redirects};}
+if(typeof route.destination==='string'){if(route.destination.startsWith('/')&&Array.isArray(sourceTokens)){const unnamedInDest=new Set();for(const token of sourceTokens){if(typeof token==='object'&&typeof token.name==='number'){const unnamedIndex=new RegExp(`:${token.name}(?!\\d)`);if(route.destination.match(unnamedIndex)){unnamedInDest.add(`:${token.name}`);}}}if(unnamedInDest.size>0){invalidParts.push(`\`destination\` has unnamed params ${[...unnamedInDest].join(', ')}`);}else{const{tokens:destTokens,error:destinationParseFailed}=tryParsePath(route.destination,true);if(destinationParseFailed){invalidParts.push('`destination` parse failed');}else{const sourceSegments=new Set(sourceTokens.map(item=>typeof item==='object'&&item.name).filter(Boolean));const invalidDestSegments=new Set();for(const token of destTokens){if(typeof token==='object'&&!sourceSegments.has(token.name)){invalidDestSegments.add(token.name);}}if(invalidDestSegments.size){invalidParts.push(`\`destination\` has segments not in \`source\` (${[...invalidDestSegments].join(', ')})`);}}}}}const hasInvalidKeys=invalidKeys.length>0;const hasInvalidParts=invalidParts.length>0;if(hasInvalidKeys||hasInvalidParts){console.error(`${invalidParts.join(', ')}${invalidKeys.length?(hasInvalidParts?',':'')+` invalid field${invalidKeys.length===1?'':'s'}: `+invalidKeys.join(','):''} for route ${JSON.stringify(route)}`);numInvalidRoutes++;}}if(numInvalidRoutes>0){if(hadInvalidStatus){console.error(`\nValid redirect statusCode values are ${[...allowedStatusCodes].join(', ')}`);}console.error();throw new Error(`Invalid ${type}${numInvalidRoutes===1?'':'s'} found`);}}async function loadRedirects(config){if(typeof config.redirects!=='function'){return[];}const _redirects=await config.redirects();checkCustomRoutes(_redirects,'redirect');return _redirects;}async function loadRewrites(config){if(typeof config.rewrites!=='function'){return[];}const _rewrites=await config.rewrites();checkCustomRoutes(_rewrites,'rewrite');return _rewrites;}async function loadHeaders(config){if(typeof config.headers!=='function'){return[];}const _headers=await config.headers();checkCustomRoutes(_headers,'header');return _headers;}async function loadCustomRoutes(config){const[headers,rewrites,redirects]=await Promise.all([loadHeaders(config),loadRewrites(config),loadRedirects(config)]);if(config.trailingSlash){redirects.unshift({source:'/:file((?:[^/]+/)*[^/]+\\.\\w+)/',destination:'/:file',permanent:true},{source:'/:notfile((?:[^/]+/)*[^/\\.]+)',destination:'/:notfile/',permanent:true});if(config.basePath){redirects.unshift({source:config.basePath,destination:config.basePath+'/',permanent:true,basePath:false});}}return{headers,rewrites,redirects};}
 //# sourceMappingURL=load-custom-routes.js.map
Daemeron commented 4 years ago

I think not being forced to opt-in into automatic trailingSlash url normalisation would be very helpful. My team wanted to upgrade to latest version of Next.js recently but this change keeps us from doing so.

Our use-case is international website that does not use trailing slashes at the end of the url except for home pages for different countries:

We would benefit from trailingSlashes: undefined (default)

Zauberfisch commented 3 years ago

related issue: #18164

And I agree, this should be optional

aralroca commented 3 years ago

Definitely, the option of not doing any redirect should be implemented.

jonathanmcchesney commented 3 years ago

This is a blocking issue for our team. We have to support a plethora of different application routes for applications we integrate with, some containing hashbang urls, some ending with trailing slashes, and some without trailing slashes. These redirects as a result are mutating our application routes which have strict route checking.

e.g.

app/1/#/some/route1 (wont work with a trailing slash) app/1/#/some/route2/ (wont work without a trailing slash) app/1/#/some/route1?theme=dark app/1/#/some/route2/?theme=dark

Is there also currently a way to optionally redirect for hashbang urls, as some of the apps we integrate with use these. At present if we go to app/1/#/some/route1, we are redirected to app/1#/some/route1.

btk commented 3 years ago

In my case, this would be helpful to preserve the SEO of the website that currently exist, that is moving to next.

Some of the current website urls have trailinglashes at the end of urls and some of them dont.

I'm not sure this is correct but, I would assume redirecting the currently indexed url structre would be an unnecessary risk.

gamb commented 3 years ago

This looks to be a blocker for me in a situation where api endpoints sometimes have a trailing slash.

jon-sully commented 3 years ago

I wonder if the more elegant solution here is to expose the ability to choose the trailing slash behavior on a route-by-route basis. The suggested behavior of setting undefined and having Next simply respond to both example.com/test/ and example.com/test with the same content may actually be quite hurtful to the site's SEO values (ref - 'Avoid duplicate content', ref - 'Avoid...pages contenting with each other', ref - Google's John Mueller) so generally I think this is something Next would probably want to avoid exposing users open to.

A route-by-route approach (with globbing / splatting) that implements an implicit inheritance-by-order may provide a more versatile approach that could solve a lot of folks' issues described in this thread (IMO). Might look something like:

E.g.

module.exports = {
  trailingSlash: {
    // Set the site default
    "/*": true,
    // Maybe the blogs are coming from a legacy WP site - retain SEO here
    "/blog/*": false,
    // @Daemeron - where the locale-specific _sub-pages_ don't use trailing
    // but the locale-specific 'landing' page does (would hit first rule)
    "/-*-/*": false
  },
}

No clue at this point how that would get implemented, but food for thought 🙂

aralroca commented 3 years ago

@jon-sully I like your proposal, but I miss some way to for example turn off trailing slash for 404 pages. To avoid 308 -> 404, returning 404 status directly.

Zauberfisch commented 3 years ago

if we we want an option to map urls individually and be highly flexible about it, then perhaps the best option would be a callback.

module.exports = {
  trailingSlash: (slug) => {
    // Maybe the blogs are coming from a legacy WP site - retain SEO here
    if (slug.match(/^\/blog\/*/)) {
      return false
    }
    if (doSomeOtherCheckHere()) {
      return ...
    }
    // Set the site default
    return true
  },
}
Zauberfisch commented 3 years ago

but then I'm wondering if a callback like this might be to much to specific. I think we should keep the trailingSlash option simpler. But add an option to set it to "do-nothing" (be it null, false, or what ever) and then another callback that is more generic. Say we call it onBeforeMatchRoute. And so in onBeforeMatchRoute developers can do custom redirects, custom slash redirects, and other stuff.

jon-sully commented 3 years ago

I think the callback with the path as an argument is a good idea, although I think the implementation of this entire premise will be tricky. I'm not as familiar with Next as I am Gatsby, but getting the trailing slash stuff working correctly requires your output static files, server configuration, client-side router (if exists), and client-side navigations to all be working in sync. Gatsby's got its own problems with it 🤣 So I guess all that to say, a path callback sounds great, but it'll be pretty hard to implement if someone wants to take that on.

andrewmunsell commented 3 years ago

To add to the valid usecases-- I have a catchall proxy endpoint that forwards requests to another Wordpress backend, so part of my site is Next.JS, with the remaining being WP.

The 308 redirects are completely breaking the WP editor because the redirects are completely non-configurable and therefore I cannot add the correct CORS headers to the redirect response. Ideally I'd just disable all redirects, because WP has a mix of trailing slash and non-trailing slash API calls that it makes.

Zauberfisch commented 3 years ago

On second thought, keep it simple, let's just add an option to disable redirects.

This way we have a quick implementation time and we can implement a more complex solution later. Also, there are other ways to implement redirects (custom app, webserver in front of next, ...) (I personally do all redirecting with apache or nginx, so I don't actually want apps to do those themselves)

armspkt commented 3 years ago

@Janpot There are many cases right now that want to config trailingSlashes not only true and false but undefined options too.

medatech commented 3 years ago

I would just like to add support for more options as we found it to be a showstopper for a client project. Our client has a mixture of existing URLs which are already ranking in SEO which use a mix of trailing slashes and non-trailing slashes. Their strategy is to gradually migrate these to be consistent while minimising the thanking changes to search engines. This means it's not possible to explicitly pick with trailing slashes or without.

The solution we've adopted involves:

  1. Using a custom next app that manages the req/res manually. This prevents the server doing automatic redirects.
  2. Not using next/link as this rewrites the target URLs, which again we don't want.

These workarounds naturally lose some benefits provided by next.

vitorbal commented 3 years ago

Chiming in with a similar use-case to @andrewmunsell..

I'm migrating from an old django system with inconsistent usage of trailing slashes for api/page routes, and using rewrites to proxy all routes my Next app doesn't handle yet back to the django system:

  async rewrites() {
    return [
      // we need to define a no-op rewrite to trigger checking
      // all pages/static files before we attempt proxying
      {
        source: "/:path*",
        destination: "/:path*",
      },
      {
        source: "/:path*",
        destination: `https://my-django-app.com/:path*`,
      },
    ];
  },

That mostly works, but there's one problem: some of the django API routes are opinionated about their trailing slashes, so will do a 301 if you try to access them without a trailing slash. That means it's hard to come up with a set of rules for my rewrites that deals with all the edge-cases, unless I start listing them out one by one.

It would be nice if we could disable next's trailing slash handling entirely for rewrites rules:

  async rewrites() {
    return [
      // we need to define a no-op rewrite to trigger checking
      // all pages/static files before we attempt proxying
      {
        source: "/:path*",
        destination: "/:path*",
      },
      {
        source: "/:path*",
        destination: `https://my-django-app.com/:path*`,
        disableTrailingSlashRedirects: true
      },
    ];
  },