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

Should /page/ work just like /page ? #1189

Closed
davibe opened this issue Feb 17, 2017 · 47 comments
Closed

Should /page/ work just like /page ? #1189

davibe opened this issue Feb 17, 2017 · 47 comments

Comments

@davibe
Copy link
Contributor

davibe commented Feb 17, 2017

I am using Google Search Console to inspect my website. When adding a website it automatically adds a "/" to the end of the url, therefore www.website.com/page becomes www.website.com/page/

In the case of next page called 'page' this would not work. There are 2 workarounds

  1. create /page/index.js on the filesystem
  2. use a custom server to solve this "/" thing

Regarding 2 this is what i have done

req.url = req.url.replace(/\/$/, "")
if (req.url == "") { req.url = "/" }
handle(req, res)

I wonder wether it makese sense to fix this in next.js itself making it more generic.

@davibe davibe changed the title Should /page/ work just like /page Should /page/ work just like /page Feb 17, 2017
@davibe davibe changed the title Should /page/ work just like /page Should /page/ work just like /page Feb 17, 2017
@davibe davibe changed the title Should /page/ work just like /page Should /page/ work just like /page ? Feb 17, 2017
@khrome83
Copy link
Contributor

khrome83 commented Feb 19, 2017

For me this makes sense.

Personally, I think having this workflow would make sense.

  1. page.js exists -> /page & /page/ works and goes to same place
  2. page/index.js exist, page.js does not -> /page would 404, /page/ would resolve at index.js
  3. page.js exists, as does page/index.js -> /page would go to page.js, /page/ would go to page/index.js

In other words /page/ would first trying index in folder, then fall back to extension /page.js

@timneutkens
Copy link
Member

kind of related #619

@rauchg
Copy link
Member

rauchg commented Feb 20, 2017

I mostly agree with your reasoning @krhome83 but your statement in point 2 (/page would 404) is not logically consistent

  1. In point 1) you suggest that a trailing slash in the URL (/page/), which typically indicates a directory, should resolve when the underlying representation is a file (./pages/page.js)
  2. In point 2) you suggest the opposite. That /page as the URL, which typically indicates a file_, should 404 when the underlying representation is a folder ./pages/page/)

@rauchg
Copy link
Member

rauchg commented Feb 20, 2017

Let me know if I'm off base, but otherwise, I think I'd like to add a "smart default" for this scenario (since we also have a custom server override for https://zeit.co), provided that we can turn it off in config (smartSlashes: false ?)

@rauchg
Copy link
Member

rauchg commented Feb 20, 2017

Also, I wonder if this should warn or be an error, since it's kind of not obvious

  1. page.js exists, as does page/index.js -> /page would go to page.js, /page/ would go to page/index.js

@rauchg
Copy link
Member

rauchg commented Feb 20, 2017

IMHO in that case we should probably error unless you turn off smartSlashes, and most people would probably use ./pages/index for what they otherwise wanted to handle in ./page

@arunoda
Copy link
Contributor

arunoda commented Feb 20, 2017

I think the best option is to do a 301 redirect to /page/ to /page

@rauchg
Copy link
Member

rauchg commented Feb 20, 2017

301 would mean permanent, so maybe not a good idea

@arunoda
Copy link
Contributor

arunoda commented Feb 20, 2017

Yep 301 I mean it.
Do we need /page/ to serve in some different way in the future?

I guess we don't.

@khrome83
Copy link
Contributor

khrome83 commented Jun 3, 2017

Agree with the 301 redirect. Nice and simple.
You want to use 301 instead of 302 because of SEO. Makes sure that google does not see two links as equivalent.

https://moz.com/learn/seo/redirection

Need to maintain that link juice!

@harshmaur
Copy link

Many people use trailing slash and many dont, however its better to only keep one version of it as per SEO otherwise it will create serious issues as per google.

We maintain a trailing slash and I am not sure how I can do it in next js.

Any ideas?

@sergiodxa
Copy link
Contributor

@harshmaur you can create a custom server to handle urls with a trailing slash or you can create pages/my-page/index.js, that way your URL (with the default router) will be /my-page/

@harshmaur
Copy link

@sergiodxa Thanks a lot, I was thinking of using server.js to create custom routes for all of my pages instead of using the default router. But I am not sure how that would work on the client side. I need to check.

@sergiodxa
Copy link
Contributor

@harshmaur check the custom server example specially how to use next/link

@weisjohn
Copy link

I believe I'm currently running into something like this right now.

Using a next/link instance, the rendered anchor element to /foo, and the static element points to /foo, but the prefetcher is taking me to /foo/, which my analytics team says is a no-go. My server is configured to serve up the /foo page perfectly. (I'm using the next export feature as well).

I believe this is the relevant line in _rewriteUrlForNextExport() https://github.com/zeit/next.js/blob/cc9b1d6ce071bc40b52326f7bb01ecb8ff0657be/lib/router/index.js#L101

@rauchg would that be the target of your proposed smartSlashes flag?

@ArmorDarks
Copy link

SEO-wise 301 redirect is definitely preferred, but should trailing slash in the end be stripped or instead enforced is a question.

Note, that besides already mentioned differences above, those paths will work differently with relative urls:

/foo/ with href=`test` will be `/foo/test`
/foo with href=`test` will be `/test`

That's because for browsers, no matter what you think and how servers resolves resources, any url with trailing slash is a directory, while without one — a document. Thus, resolution for relative resources will be different. This means that by enforcing certain behavior (trailing slash or lack of one) you effectively removing a way to treat relative urls in different ways.

This shouldn't be a real issue, since, as far as I remember, in react-router relative paths are not even supported. But still worth mentioning.

Besides, unfortunately, SEO-wise 301 redirect is quite recommended thing for ending slashes. Or, otherwise, at least, canonical urls have to be specified.

@elmasse
Copy link
Contributor

elmasse commented Aug 24, 2017

I have been looking at this issue too. One thing worth to mention is that when exporting a page as an html endpoint such as the following causes an extra backslash:

{
  exportPathMap: () => ({
          "/about.html": { page: "/about" }
  })
}

This makes simple things like an anchor not to work properly since

http://localhost:8080/about.html#contact

is not the same as

http://localhost:8080/about.html/#contact

@elmasse
Copy link
Contributor

elmasse commented Aug 26, 2017

@arunoda @sergiodxa would you guys consider a PR for avoiding the extra backslash in _rewriteUrlForNextExport if the path ends with .html?

@hmontes
Copy link

hmontes commented Aug 26, 2017

I have the same issue. I'm applying "with-apollo-and-redux" official example. https://github.com/zeit/next.js/tree/master/examples/with-apollo-and-redux

I created a pages/test. If i browse localhost:3000/test all it's okey. But if i browse localhost:3000/test/

GET http://localhost:3000/test/ 404 (Not Found)
client.js?ecc360b:67 [HMR] connected
warning.js?1713c82:35 Warning: Failed prop type: The prop `serverState` is marked as required in `WithData(Test)`, but its value is `undefined`.
    in WithData(Test) (created by Container)
    in Container (created by App)
    in div (created by App)
    in App

But the page loads (With all the messages in the console)

@hmontes
Copy link

hmontes commented Aug 29, 2017

Maybe a temp fix.

server.js

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

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

app.prepare()
.then(() => {
  createServer((req, res) => {
    const parsedUrl = parse(req.url, true)
    const { pathname, query } = parsedUrl

    if (pathname.length > 1 && pathname.slice(-1) === '/') {
      app.render(req, res, pathname.slice(0, -1), query)
    }
    else {
      handle(req, res, parsedUrl)
    }

  })
  .listen(port, (err) => {
    if (err) throw err
    console.log(`> Ready on http://localhost:${port}`)
  })
})

package.json

...
"scripts": {
    "dev": "node server.js",
    ...
  },
....

Now this works good except for an error in the first render.

process-update.js?0be4961:122 [HMR] Update check failed: Error: Manifest request to /_next/webpack/53918e2236aadcc582cd.hot-update.json timed out.
    at XMLHttpRequest.request.onreadystatechange (http://localhost:3000/_next/1504017609143/manifest.js:68:22)

@ritz078
Copy link

ritz078 commented Sep 1, 2017

@hmontes But this won't be a fix for static exports.

@flippidippi
Copy link

I would love an internal fix for this that works with next-routes.

@timneutkens
Copy link
Member

Fixed in next 5 / canary

@adekbadek
Copy link

url with a trailing slash results in a 404 error using next@5.1.0, was it supposed to be fixed in this version?

@jabacchetta
Copy link

@timneutkens Should this be reopened?

@jabacchetta
Copy link

jabacchetta commented Apr 18, 2018

One other similar issue is support for multiple forward slashes, as it's easy for users to miss two of them. Example being: https://github.com/zeit/next.js////////////issues/1189

@lkbr
Copy link

lkbr commented Apr 27, 2018

Please see #3876 (comment), where I've shared a minimal example where trailing slashes don't seem to be working.

@NathanielHill
Copy link
Contributor

For reference:
https://zeit.co/blog/new-static-deployments

@lkbr
Copy link

lkbr commented Jun 14, 2018

Isn't that just for static / next export though?

@iamstarkov
Copy link

I filed a new issue with minimal reproducible repository in #5214

@isaachinman
Copy link
Contributor

What is the status on this issue? It's been open for quite some time. I've just noticed that /page/ will still result in a 404. I'm using v7.0.1.

@lvnilesh
Copy link

Using v7.02, I have a page.js so /page works but /page/ results in a 404.

What's the workaround?

@isaachinman
Copy link
Contributor

For what it's worth, I've implemented a force-trailing-slash util at next-i18next to circumvent this issue. In the meantime I would suggest people implement their own middlewares.

@rap2hpoutre
Copy link

@timneutkens Hi, should this issue be re-opened? It still does not work with current version

@ryandcsg

This comment has been minimized.

@rap2hpoutre
Copy link

rap2hpoutre commented Jun 28, 2019

ping @timneutkens it seems many people are upvoting to reopen. Any plan to re-open it?

@dryleaf
Copy link

dryleaf commented Jul 15, 2019

@rap2hpoutre What is the status of this issue?

@rap2hpoutre
Copy link

@dryleaf Still no answer. Maybe we should open a new issue.

@iamstarkov
Copy link

@rap2hpoutre it is already opened #5214

@GabrielDelepine
Copy link

GabrielDelepine commented Mar 13, 2020

Seams fixed.

The page is accessible with or without trailing slash. Both returning HTTP 200.

I am using Next.js v9.3.0

@javiercno
Copy link

Seams fixed.

The page is accessible with or without trailing slash. Both returning HTTP 200.

I am using Next.js v9.3.0

I am using Next.js v9.3.0 and not works

@iamstarkov
Copy link

@javiercno see #5214

@untitledlt
Copy link

Can we reopen this?
I still can not match route with trailing slash.
Using 9.3.1 with custom server.

@timneutkens is there a hope this could be fixed any time soon?

@iamstarkov
Copy link

@untitledlt see #5214

@vikrantraut
Copy link

Seams fixed.

The page is accessible with or without trailing slash. Both returning HTTP 200.

I am using Next.js v9.3.0

I am using Next.js v9.3.0 and not works.can we reopen this bug?

@untitledlt
Copy link

@iamstarkov i saw your issue. There's even my comment 3 days ago.

@timneutkens
Copy link
Member

timneutkens commented Mar 24, 2020

As said this is already open in #5214

Going to lock this issue as there's no actionable comments here.

@vercel vercel locked as off-topic and limited conversation to collaborators Mar 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests