search

vercel / next.js

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

trailing slash in link for legit page works for client side navigation but leads to not found bundle and 404 on hard refresh (ssr) #5214

Closed iamstarkov closed 4 years ago

iamstarkov commented 6 years ago

trailing slash in link for legit page works for client side navigation but leads to not found bundle and 404 on hard refresh (ssr)

Bug report

Describe the bug

let me know if title needs further clarification.

all relevant issues has been closed with reasoning that its been fixed in 6-canary (I believe it is not) or by improved serve (which is true only in perhaps production static export).

I'm rewriting my existing blog to next.js and i previously used trailing slashes. Latest serve can help with it once i build my next.js powered blog. But in order to fix dev env i need either to get rid of trailing slashes and utilize 301 Moved Permanently in prod; or live with broken trailing slash support in dev.

To Reproduce

Here is minimal reproducible case (link to repro repo is below snippet):

// pages/index.js
import Link from "next/link";

export default () => (
  <Link href="/about/">
    <a>About</a>
  </Link>
);

// pages/index.js
export default () => "about";

Minimal reproducible repo https://github.com/iamstarkov/next.js-trailing-slash-bug-demo

  1. clone repo git clone https://github.com/iamstarkov/next.js-trailing-slash-bug-demo
  2. change directory cd next.js-trailing-slash-bug-demo
  3. install deps yarn
  4. run dev: yarn dev
  5. open http://localhost:3000/
  6. open devtools' network tab
  7. observe http://localhost:3000/_next/static/development/pages/about.js being 200ed
  8. observe http://localhost:3000/_next/on-demand-entries-ping?page=/about/ being 200ed
  9. observe http://localhost:3000/about/ being 404ed
  10. observe persistent attempts to resolve http://localhost:3000/about/
  11. observe in the terminal Client pings, but there's no entry for page: /about/
  12. refresh the page
  13. observe 404 page.
  14. remove trailing slash in the url or click http://localhost:3000/about
  15. observe page being 200ed
  16. to ensure error persistence repeat steps 5-15 once.

Expected behavior

  1. /about/ shouldnt be resolved as 404 not found
  2. /about/ should be resolved as 200 ok
  3. Server should not print Client pings, but there's no entry for page: /about/
  4. both /about and /about/ should work the same way

Screenshots

N/A

System information

Additional context

Add any other context about the problem here.

If you change this code in https://github.com/zeit/next.js/blob/459c1c13d054b37442126889077b7056269eeb35/server/on-demand-entry-handler.js#L242-L249

or node_modules/next/dist/server/on-demand-entry-handler.js locally

          const { query } = parse(req.url, true)
          const page = normalizePage(query.page)
+         console.log('query.page', query.page);
+         console.log('page', page);
+         console.log('Object.keys(entries)', Object.keys(entries));
          const entryInfo = entries[page]

          // If there's no entry.
          // Then it seems like an weird issue.
          if (!entryInfo) {
            const message = `Client pings, but there's no entry for page: ${page}`

and restart next dev and open http://localhost:3000/ and click about link then:

I think the problem (at least part of it) is in inability of onDemandEntryHandler's middleware to find page in entries if page has trailing slash.

I hope my 2 hours of investigation and preparation can help with fixing this issue.

iamstarkov commented 6 years ago

most relevant and notable issues are #1189 and #3876

NathanielHill commented 6 years ago

Looking forward to this being finally resolved! @timneutkens What's the status of trailing slash issues for Next 7?

iamstarkov commented 6 years ago

@NathanielHill I could reproduce it on next@7

paintedbicycle commented 6 years ago

I'm using nextjs 7 and trailing slash is producing a 404 for me on both dev and prod:

And affects:

Simply removing the trailing slash fixes the issue.

Trailing slashes are often added by browsers, servers and/or other services where links might be pasted so while I can control internal links, it's hard to control what at what links external users might be arriving on

emplums commented 6 years ago

I'm also seeing this issue in version 7. Not sure if this is relevant but, I'm aliasing one Next.js project to a subfolder of another Now deployment. So our base url is primer.style and we are aliasing our primer-components.now.sh Next.js app to primer.style/components. On production, primer.style/components's index page works fine, but primer.style/components/ produces a 404.

chibicode commented 6 years ago

I had to search around a bit to find this issue. I use static deployments on Netlify so it's not an issue on prod, but on development (Next 7) the compilation just freezes if there was a trailing slash, and it was hard to figure out why. I don't think this (not handling trailing slash on dev environment) is a good DX.

ravener commented 6 years ago

Im also having this issue and it is really annoying, i hope it is fixed soon.

aluminick commented 5 years ago

If you want trailing slash, you can just do this. <Link href='/about' as='/about/'><a>about</a></Link> but if you're using fridays/next-routes this is not possible. So I have a fork where you can add trailingSlash as prop. Hope this helps

iamstarkov commented 5 years ago

If you want trailing slash, you can just do this. <Link href='/about' as='/about/'><a>about</a></Link> but if you're using fridays/next-routes this is not possible. So I have a fork where you can add trailingSlash as prop. Hope this helps

@aluminick I'm sorry, I just tried this and it doesnt work for me. I still get to traling-slashed page (latest release), which is not found after refresh (current behavior).

iamstarkov commented 5 years ago

also neither #6664 nor #6752 help with these, because experimental.exportTrailingSlash doesnt help because it is for next export only, I believe

iamstarkov commented 5 years ago

there was a promising pull request #6421 by @Janpot which didn't reach any consensus, unfortunately

dryleaf commented 5 years ago

@iamstarkov What's the status of this issue? Any solutions beside the server.js hook?

iamstarkov commented 5 years ago

@dryleaf status: it is still opened

jabacchetta commented 5 years ago

A similar issue... redirect when multiple forward slashes are added. Example: https://github.com/zeit/next.js////////////issues/5214

iamstarkov commented 5 years ago

GitHub urls are irrelevant

jabacchetta commented 5 years ago

@iamstarkov Not sure what you mean. But after rereading my original post, it looks like I could have been more clear.

The GitHub url is meant to be a simple demonstration of how urls should (preferably) work when an app is built with Next.js. In other words, if a user adds an extra slash, the url should still work.

farhadniaz commented 5 years ago

Any update for nextjs 9 ?

nik-john commented 5 years ago

I'm new to Next but what is the workaround you folks are using for this issue?

pavelzubov commented 5 years ago

@iamstarkov What's the status of this issue?

exentrich commented 5 years ago

I'm shocked that this issue not solved in any way for about year! Do Next.js team need any other reasons to start fixing this?

URL's should work regardless of trailing slash. Check any site on the web.

If this is out of scope of Next.js, give us ability to configure this in Now. I'm really confused that Zeit team ignores such critical issues for years.

NathanielHill commented 5 years ago

@exentrich This is easily configurable in Zeit Now by simply 301 redirecting all trailing slashes to the same route without slashes:

now.json:

"routes": [
    {
      "src": "/(.*)/",
      "status": 301,
      "headers": { "Location": "/$1" }
    },
    ...
]

However, I also don't understand why this is not handled by Next.js itself and why the team has ignored this issue.

This, along with a public/ (in the works) are the main issues I see CRA converts running in to.

@rauchg

exentrich commented 5 years ago

@NathanielHill thanks! I tried this solution, but query parameters are stripped out. For example /some/?query=1 will redirect to /some without query. Do you know how to fix it?

NathanielHill commented 5 years ago

Yeah, that sounds like a problem @exentrich

I wouldn't have guessed that behavior as I've been told there's an implicit ^ and $ wrapped around the regex (meaning your example wouldn't match). Maybe there's a way to access the query string on it's own to add it back :man_shrugging: Good luck

dkrish commented 5 years ago

Trying to get it working using a custom express server and avinoamr/connect-slashes but seem to be running into the same issue

nik-john commented 5 years ago

This is certainly a massive issue especially because / routes throw Error pages and that hurts SEO (which is one of the primary draws of Next).

The 301 redirects and the custom express servers all seem to be hacks rather than fixes. In my case, I have a full working application built on Next with no custom Express server - everything else works perfectly, but now I'm having to create a new Express server only because of the trailing slash issue. The effort required seems to be disproportionate considering this is a hack. I would love if this could be bumped up in priority! Due to this reason, I'm hearing grumblings in my team about having used Next as opposed to something like vanilla React/Angular and it certainly weakens the case for Next.

PS: I absolutely love working with Next ❤️

Janpot commented 5 years ago

This is certainly a massive issue especially because / routes throw Error pages and that hurts SEO

It doesn't hurt your SEO. google treats trailing slash as a different page. Having it 404 doesn't impact SEO any more than any other non existing page in your site. Besides, as long as you never link to it with a trailing slash, google won't try to crawl it in the first place. This issue, while still being a valid issue, is way less critical than you all make it to be.

brianhiss commented 5 years ago

@nik-john @NathanielHill @dkrish @exentrich

You shouldn't have to use an Express server to do a 301 Redirect. Depends on your requirements, but I've been able to meet mine with a custom server.js.

A 301 redirect is also the best way to go for SEO, as you won't get duplicate content penalties for the slash and non-slash route.

I love ❤️ Next.js, but I vote for this to be handled without this work around.

// server.js

const { createServer } = require('http');
const { parse } = require("url");
const next = require("next");

const dev = process.env.NODE_ENV !== 'production'
const port = parseInt(process.env.PORT, 10) || 3000;
const app = next({ dev, quiet: false });
const handle = app.getRequestHandler();

(async () => {
    await app.prepare();
    const server = createServer();

    server.on('request', async (req, res) => {

        const parsedUrl = parse(req.url, true);
        const { pathname, query } = parsedUrl;

        if (pathname.length > 1 && pathname.slice(-1) === "/") {
            console.log('server.js - redirect on "/"...', pathname, query);
            const queryString = await Object.keys(query).map(key => key + '=' + query[key]).join('&');
            res.writeHead(301, { Location: pathname.slice(0, -1) + (queryString ? '?'+ queryString : '') });
            res.end();
        }

        handle(req, res, parsedUrl);

    });

    await server.listen(port);
    console.log(`🚀 Ready on http://localhost:${port}`);

})();
nik-john commented 5 years ago

@Janpot

It doesn't hurt your SEO. google treats trailing slash as a different page. Having it 404 doesn't impact SEO any more than any other non existing page in your site.

I take your point that it doesn't particularly hurt SEO innately. But it does put additional pressure on developers to get the URL definitions right every time, which is subject to human errors. A developer that is new to Next wouldn't necessarily know that the following (perfectly normal looking) URL will lead to a 404 page. <Link href='/people/'>

A mature framework shouldn't be subject to such human errors ideally imo.

Besides, as long as you never link to it with a trailing slash, google won't try to crawl it in the first place.

Again - there exists the issue of people accidentally linking to the www.mysite.com/people/ instead of www.mysite.com/people (both of which seem to be exactly the same for users - even most developers).

Both of these scenarios can affect SEO.

Now, not considering the SEO impact, there's also the semantic meaning of the URL - what does www.mysite.com/people/ point to? Ideally because it is pointing to a directory, Next should return whatever is in pages > people > index.js (as opposed to pages > people.js for www.mysite.com/people) but instead it returns nothing, which is a very high level flaw in how the routing works.

Major routing libraries already have some provision for this - like isExact in the case of React Router

While I understand where you're coming from, I still think this is a glaring issue that needs to be bumped up

NathanielHill commented 5 years ago

This is also completely unavoidable in the case of next export

Janpot commented 5 years ago

there exists the issue of people accidentally linking...

There exists the issue of people accidentally linking to any non-existing url, why would/some/path/ be less non-existing than /some/path/dhgfiuwo?

there's also the semantic meaning of the URL

This is highly subjective, as far as I know there is no spec out there that dictates what is the semantic difference. According to the URL spec, with and without trailing slash are considered different urls. I can think of at least 7 different valid behaviours:

Pair this with the possibility of having either /pages/some-page.js and /pages/some-page/index.js (or both).

Should next.js support all those use cases? Should it pick a default behaviour?

I'm not against this, but after trying to implement this before, I just think there's more nuance to it than it initially seems.

pavelzubov commented 5 years ago

There exists the issue of people accidentally linking to any non-existing url, why would/some/path/ be less non-existing than /some/path/dhgfiuwo?

For case /some/path/dhgfiuwo - people expect tha dhgfiuwo route can to be missing. (For example, user dhgfiuwo cannot be found in system and way users/dhgfiuwo is wrong. The absence of a user in the system is an expected occurrence.) For case /some/path/ - people expect this path to be same as /some/path, because this is default behavior on other sites. Therefore, a failure in would/some/path/is less non-existing than /some/path/dhgfiuwo.

DevSpeak commented 5 years ago

I see others have posted their solutions, so I wanted to share my approach : https://github.com/DevSpeak/next-trailingslash

Some improvements and support for dynamic routed pages when it comes to ?= should be done IMO, but this is only for showing the idea.

bitjson commented 5 years ago

For a quick solution, you can replace the default _error page (as in @DevSpeak's example).

@DevSpeak, I'd recommend a few changes for your repo:

Here's what I'm using in a Typescript project (based on the built-in error page):

/pages/_error.tsx (or remove the TypeScript types and name it /pages/_error.jsx):

import React from 'react';
import Head from 'next/head';
import { NextPageContext } from 'next';

const statusCodes: { [code: number]: string } = {
  400: 'Bad Request',
  404: 'This page could not be found',
  405: 'Method Not Allowed',
  500: 'Internal Server Error'
};

export type ErrorProps = {
  statusCode: number;
  title?: string;
};

/**
 * `Error` component used for handling errors.
 */
export default class Error<P = {}> extends React.Component<P & ErrorProps> {
  static displayName = 'ErrorPage';

  static getInitialProps({
    req,
    res,
    err
  }: NextPageContext): Promise<ErrorProps> | ErrorProps {
    const statusCode =
      res && res.statusCode ? res.statusCode : err ? err.statusCode! : 404;
    if (typeof window === 'undefined') {
      /**
       * Workaround for: https://github.com/zeit/next.js/issues/8913#issuecomment-537632531
       * Test vectors:
       * `/test/test/` -> `/test/test`
       * `/test/////test////` -> `/test/test`
       * `/test//test//?a=1&b=2` -> `/test?a=1&b=2`
       * `/test///#test` -> `/test#test`
       */
      const correctPath = (invalidPath: string) =>
        invalidPath
          .replace(/\/+$/, '')
          .replace(/\/+#/, '#')
          .replace(/\/+\?/, '?')
          .replace(/\/+/g, '/');
      if (req && res && req.url && correctPath(req.url) !== req.url) {
        res.writeHead(302, {
          Location: correctPath(req.url)
        });
        res.end();
      }
      const reqInfo = req
        ? `; Url: ${req.url}; IP: ${req.headers['x-forwarded-for'] ||
            (req.connection && req.connection.remoteAddress)};`
        : '';
      console.log(`Error rendered: ${statusCode}${reqInfo}`);
    }
    return { statusCode };
  }

  render() {
    const { statusCode } = this.props;
    const title =
      this.props.title ||
      statusCodes[statusCode] ||
      'An unexpected error has occurred';

    return (
      <div style={styles.error}>
        <Head>
          <title>
            {statusCode}: {title}
          </title>
        </Head>
        <div>
          <style dangerouslySetInnerHTML={{ __html: 'body { margin: 0 }' }} />
          {statusCode ? <h1 style={styles.h1}>{statusCode}</h1> : null}
          <div style={styles.desc}>
            <h2 style={styles.h2}>{title}.</h2>
          </div>
        </div>
      </div>
    );
  }
}

const styles: { [k: string]: React.CSSProperties } = {
  error: {
    color: '#000',
    background: '#fff',
    fontFamily:
      '-apple-system, BlinkMacSystemFont, Roboto, "Segoe UI", "Fira Sans", Avenir, "Helvetica Neue", "Lucida Grande", sans-serif',
    height: '100vh',
    textAlign: 'center',
    display: 'flex',
    flexDirection: 'column',
    alignItems: 'center',
    justifyContent: 'center'
  },

  desc: {
    display: 'inline-block',
    textAlign: 'left',
    lineHeight: '49px',
    height: '49px',
    verticalAlign: 'middle'
  },

  h1: {
    display: 'inline-block',
    borderRight: '1px solid rgba(0, 0, 0,.3)',
    margin: 0,
    marginRight: '20px',
    padding: '10px 23px 10px 0',
    fontSize: '24px',
    fontWeight: 500,
    verticalAlign: 'top'
  },

  h2: {
    fontSize: '14px',
    fontWeight: 'normal',
    lineHeight: 'inherit',
    margin: 0,
    padding: 0
  }
};

Note, this also logs an error when the page is hit, so you can check your logs to fix any links/other issues.

nik-john commented 5 years ago

@DevSpeak @bitjson Thanks for your suggestions. That's certainly one way to go about this and certainly solves the issue very well. But considering that the _error.jsx is originally meant to handle errors and not house routing logic, in my opinion having all of this code there is hacky and quite declarative. Expecting every user to do this in every code base shouldn't be a requirement - this should come out of the box. = I am of the opinion that this condition needs to be built in with the routing logic, with an option to opt out like React Router.

@NathanielHill

This is also completely unavoidable in the case of next export

Wait - I understood from reading the documentation that there is specific code to handle the trailing slash condition:

The pages will be exported as html files, i.e. /about will become /about.html.

It is possible to configure Next.js to export pages as index.html files and require trailing slashes, i.e. /about becomes /about/index.html and is routable via /about/. This was the default behavior prior to Next.js 9. You can use the following next.config.js to switch back to this behavior:

// next.config.js
module.exports = {
exportTrailingSlash: true,
}

Even if this isn't really an option for static HTML export via next export, I don't agree with the logic that just because Next supports this (amazing) feature, other modes need to suffer (I don't know the usage stats but I would assume more people use the regular with-server mode as opposed to serverless) especially when this has been known to be such a common use case

Janpot commented 5 years ago

FYI: There's an RFC that might interest you https://github.com/zeit/next.js/issues/9081

// next.config.js
module.exports = {
  async redirects() {
    return [
      {
        source: "/:path*/",
        destination: "/:path",
        statusCode: 301
      }
    ];
  }
};
nik-john commented 5 years ago

@Janpot Love it - this will bring us half-way i.e. have some sort of support for redirects without having to create a custom server. This is still going to be imperative because for every route the user adds, they would have to set up a redirect in the next.config.js - or maybe we could just use a regex to catch all cases like @bitjson mentioned:

invalidPath
          .replace(/\/+$/, '')
          .replace(/\/+#/, '#')
          .replace(/\/+\?/, '?')
          .replace(/\/+/g, '/')

In either case, if the core team is prioritizing this RFC, I would highly recommend we go one step further and make it a built in config that one can opt out of like so

// next.config.js
module.exports = {
  ignoreStrictRoutes: false, // default value: true
};

All in all, I think this is a great step forward - good stuff @Timer!! 🔥

Janpot commented 5 years ago

@nik-john The path I specified in "/:path*/" Should catch all (:path catches a single segment, * makes it catch 0 to n instances.)

nik-john commented 5 years ago

@Janpot Ah my bad 🤦‍♂ I am guessing we would also need to consider any trailing query params in that regex

Also, I still stand by the second part though:

In either case, if the core team is prioritizing this RFC, I would highly recommend we go one step further and make it a built in config that one can opt out of like so

// next.config.js
module.exports = {
  ignoreStrictRoutes: false, // default value: true
};
maerzhase commented 5 years ago

If you are using a custom server and want to ignore strict routes you can also use a custom route handler instead of making a redirect. 

app.render(req, res, urlWithoutTrailingSlash, query);

This way we are able to support both /path and /path/ and resolve to the same page.

ajhool commented 5 years ago

Oauth federation providers often require trailing forward slashes so this behavior makes a simple flow very complicated. What is the technical challenge in implementing this behavior? Or is this a design decision from next?

I haven't seen it mentioned thus far in this thread, but I am not experiencing this issue after deployment with Now, I'm only experiencing it locally when testing with now dev.

GaneshKathar commented 5 years ago 
const removeTrailingSlashes = (req, res, expressNext) => {
  if (req.path.substr(-1) === '/' && req.path.length > 1) {
    const query = req.url.slice(req.path.length);
    res.redirect(301, req.path.slice(0, -1) + query);
  } else {
    expressNext();
  }
};

got this from stackoverflow and worked perfectly. this solution work with express.

iamstarkov commented 5 years ago

@GaneshKathar I don't see how this will work if you account for Next.js not using express

Ciantic commented 5 years ago

I think we can't agree on this and it should be configurable.

I actually want the trailing slash always, the relative urls are easier to reason about when all pages end with trailing slash.

For instance it makes no sense that /about/index.tsx is /about instead of /about/, but understandable now that next expects without trailing slash. If all pages were to end in slash it would allow pages to contain subpages in the future, which I think is more extensible way for pages.

Making relative links inside /about/index.tsx file is now cumbersome. If you make a link ./mysubpage/ it points to root of the site instead. This makes the subpages non-renameable. I can't make a directory /about/ full of pages that I can just rename, because I should go and edit the relative links too.

Also the wget -r site produces sensible results with having always trailing slashes, producing index.html files.

However changing this setting is massively breaking change since all sites expect non-trailing slashes, so it must be configurable.

xe4me commented 5 years ago

I'm using version 9 and this issue is still not resolved

rafaeleyng commented 4 years ago

I was able to make it work by using something like the following on my next.config.js:

exportPathMap: async function() {
  const paths = {
    '/': { page: '/' },
    '/authors/index.html': { page: '/authors' },
  };

  return paths;
},

Accessing /authors gives 302 pointing location to /authors/. I'm testing with http-serve, not sure if this behavior is server-specifiy.

cnblackxp commented 4 years ago

when I faced this issue I came up with this solution

in my _error.js page

Error.getInitialProps = ({ res, err, asPath }) => {
    const statusCode = res ? res.statusCode : err ? err.statusCode : 404;

    const checkForTrailingSlashes = () => {
        if (asPath.match(/\/$/)) { // check if the path ends with trailing slash
            const withoutTrailingSlash = asPath.substr(0, asPath.length - 1);
            if (res) {
                res.writeHead(302, {
                    Location: withoutTrailingSlash
                })
                res.end()
            } else {
                Router.push(withoutTrailingSlash)
            }
        }
    }

    if (statusCode && statusCode === 404) {
        checkForTrailingSlashes();
    } else {
        // 
    }
    return { statusCode };
}

is it a good way to overcome the issue ?

AlexSapoznikov commented 4 years ago

How about this?

pages/_app.jsx

import App from 'next/app';

export default class MyApp extends App {
  render() {
    const { Component, pageProps, router: { asPath } } = this.props;

    // Next.js currently does not allow trailing slash in a route.
    // This is a client side redirect in case trailing slash occurs.
    if (asPath.length > 1 && asPath.endsWith('/')) {
      const urlWithoutEndingSlash = asPath.replace(/\/*$/gim, '');

      if (typeof window !== 'undefined') {
        window.location.replace(urlWithoutEndingSlash);
      }
      return null;
    }

    return <Component {...pageProps} />;
  }
}
cansin commented 4 years ago

@cnblackxp thanks for the suggestion. That helped me. Here is how I implemented it in order to keep the default behavior for non-trailing 404s (i.e. I am simply re-exporting the default Error implementation):

import Error from "next/error";
import Router from "next/router";

export default Error;

Error.getInitialProps = ({ res, err, asPath }) => {
  const statusCode = res ? res.statusCode : err ? err.statusCode : 404;

  if (statusCode && statusCode === 404) {
    if (asPath.match(/\/$/)) {
      const withoutTrailingSlash = asPath.substr(0, asPath.length - 1);
      if (res) {
        res.writeHead(302, {
          Location: withoutTrailingSlash
        });
        res.end();
      } else {
        Router.push(withoutTrailingSlash);
      }
    }
  }

  return { statusCode };
};
bherzig-nist commented 4 years ago

yep that'll do @cansin as long as nothing else is decided :) cheers!

mbrowne commented 4 years ago

Small improvement to @AlexSapoznikov's workaround:

  render() {
    const { Component, pageProps, router: { asPath } } = this.props;

    // Next.js currently does not allow trailing slash in a route.
    // This is a client side redirect in case trailing slash occurs.
    if (pageProps.statusCode === 404 && asPath.length > 1 && asPath.endsWith('/')) {

The only difference here is checking that the status code is 404. I ran into issues using Link for dynamic routes where they were always rendering on the server because of the redirect. If you want client-side routing to work, you can't add a trailing slash to the Link href prop, but then you need to make sure you don't redirect in this case.