feat(builder, router): enforce uniform trailing slash handling #6331

manniL · 2019-08-29T00:54:22Z

Types of changes

Bug fix (a non-breaking change which fixes an issue)
New feature (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

Trailing slash handling is important when it comes to SEO. For every existing Nuxt application, the content provided by /route and /route/ should be the same (opposed to how it sometimes worked back in the days). But when it comes to how the search engine sees our page, we have to be consistent and choose one of the options, either with or without a trailing slash.

Be consistent with the preferred version. Use it in your internal links. If you have a Sitemap, include the preferred version (and don’t include the duplicate URL).

With the current setup, it is not entirely possible to enforce trailing slashes (or the removal of them) without substantial effort.

As long as you have simple and fixed URLs, you can always append the slash (or not), but when it comes to resolved named routes (e.g. when using https://github.com/nuxt-community/nuxt-i18n/ ), things become difficult and hard to manage.

Thus, this PR

It introduces a new router option called trailingSlashes set to undefined by default. If it's set to true, trailing slashes will be appended on every route. If set to false, they'll be removed.

Therefore, we use the pathToRegexpOptions settings (per route) provided via vue-router (see vuejs/vue-router#1753 and vuejs/vue-router#1443)

This option should not be taken lightly and has to be tested thoroughly. When setting router.trailingSlashes to something else than undefined, one of the options (either with or w/o trailing slash) will stop working. 301 redirects should be in place before.

Related work:

#3448
#5746

TODO

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly. (PR: #)
I have added tests to cover my changes (if not applicable, please state why)
All new and existing tests are passing.

codecov-io · 2019-08-29T01:04:57Z

Codecov Report

Merging #6331 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             dev   #6331      +/-   ##
========================================
+ Coverage   95.7%   95.7%   +<.01%     
========================================
  Files         79      79              
  Lines       2652    2656       +4     
  Branches     683     685       +2     
========================================
+ Hits        2538    2542       +4     
  Misses        98      98              
  Partials      16      16

Flag	Coverage Δ
#e2e	`100% <ø> (ø)`	⬆️
#fixtures	`50.56% <50%> (-0.01%)`	⬇️
#unit	`92.35% <100%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
packages/config/src/config/router.js	`100% <ø> (ø)`	⬆️
packages/utils/src/route.js	`100% <100%> (ø)`	⬆️
packages/builder/src/builder.js	`99.62% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9c4bcf...1125ba4. Read the comment docs.

galvez

Good idea.

pimlie · 2019-08-29T08:05:55Z

Nice timing, have been struggling with trailing slashes in nuxt/press a bit the last weeks!

I'd recommend to include a util method so users can easliy normalize paths they use in their project as well, see eg here for the methods currently used in nuxt/press (but they could be improved on probably).

atinux · 2019-08-29T10:18:28Z

Good call!
Shouldn’t be easier to directly add a meta canonical directly so we fix the duplicate for SEO? (This is what we use for nuxtjs.org)

manniL · 2019-08-29T20:19:42Z

Shouldn’t be easier to directly add a meta canonical directly so we fix the duplicate for SEO? (This is what we use for nuxtjs.org)

@atinux

Well, canonical is one thing and easily realizable via layouts (example). But the internal link structure should reflect trailing slashes (or none) as well

manniL · 2019-08-29T20:22:02Z

I'd recommend to include a util method so users can easliy normalize paths they use in their project as well

@pimlie

What would be the use case when all (named) routes or those resolved programmatically will go in line with the config and fixed routes would be easy to change by appending/removing the /?

pimlie · 2019-08-30T08:15:11Z

@atinux Isnt that technically more a workaround then a fix? Due to the nature of a SPA the path resolve behaviour is just different then serving static files with a webserver (ie apache/nginx). How close do we want to stay to that 'original' behaviour?

@manniL If your router config lists all routes with a trailing slash, then your nuxt/router-links should point to a link with a trailing slash as well right?

pi0

Nice improvement 👍

packages/utils/src/route.js

pi0 · 2019-08-30T14:10:39Z

packages/utils/src/route.js

+      route.path = route.path.endsWith('/') ? route.path : `${route.path}/`
+    }
+
+    if (trailingSlashes === false) {


Maybe we can make at an else if? This would save branch check-in cases that trailingSlashes is true + makes it more readable.

How about ?

if(trailingSlashes !== undefined) { const trailing = trailingSlashes ? '/' : '' route.path = route.path.replace(/\/+$/, '') + trailing }

pi0 · 2019-08-30T14:11:43Z

packages/utils/src/route.js

@@ -136,7 +136,8 @@ export const createRoutes = function createRoutes ({
  srcDir,
  pagesDir = '',
  routeNameSplitter = '-',
-  supportedExtensions = ['vue', 'js']
+  supportedExtensions = ['vue', 'js'],
+  trailingSlashes = undefined
 }) {
  const routes = []
  files.forEach((file) => {


(Not related to this PR) what do you think of refactoring forEach to ES6 for of? OR using files.map to directly assign the result of routes instead of pushing to the end.

Plus one for using map instead ☺️

Let's do this later and not blocking this PR :)

packages/utils/src/route.js

packages/config/src/config/router.js

packages/utils/src/route.js

clarkdo · 2019-08-30T16:02:01Z

packages/utils/src/route.js

+      route.path = route.path.endsWith('/') ? route.path : `${route.path}/`
+    }
+
+    if (trailingSlashes === false) {


How about ?

if(trailingSlashes !== undefined) { const trailing = trailingSlashes ? '/' : '' route.path = route.path.replace(/\/+$/, '') + trailing }

manniL · 2019-08-30T16:08:02Z

If your router config lists all routes with a trailing slash, then your nuxt/router-links should point to a link with a trailing slash as well right?

Correct. If you use fixed URLs as in to="/simple-string/" you can do it on your own. If you used named routes to="{ name: 'foobar' }" it is handled by vue-router already and will include the trailing slash (or not)

packages/utils/src/route.js

pimlie · 2019-08-30T18:04:35Z

If you use fixed URLs as in to="/simple-string/" you can do it on your own

Thanks, this is why I recommend a normalization utility. Maybe even within nuxt-link itself, but should also be accessible outside for easy use in eg the sitemap-module.

If we support this feature switching between having a trailing slash or not should be as effortless as possible, preferable even with just toggling the setting if you use the normalization utility everywhere.

Co-Authored-By: Xin Du (Clark) <[email protected]>

manniL · 2019-08-31T16:51:05Z

Thanks, this is why I recommend a normalization utility. Maybe even within nuxt-link itself, but should also be accessible outside for easy use in eg the sitemap-module.

Now I got it, that'd be a nice DX improvement and also guarantee that all modules implement the same normalization.

A utility directly included in the nuxt-link component is also a nice idea, but could be more difficult than it seems on the first look (taking e.g. query params and programmatic usage into consideration)

pi0 · 2019-09-01T14:37:44Z

TODO for myself: add multi scope handling for release-notes generator!

initial work

698cc59

manniL added the WIP label Aug 29, 2019

manniL mentioned this pull request Aug 29, 2019

Trailing slash option nuxt-modules/i18n#422

Closed

test: fix fixtures

ba2ceef

manniL requested review from kevinmarrec, clarkdo, pi0, aldarund, atinux, galvez, pimlie and a team August 29, 2019 01:02

galvez previously approved these changes Aug 29, 2019

View reviewed changes

pi0 reviewed Aug 30, 2019

View reviewed changes

refactor: nested ternary

7a784b5

manniL dismissed galvez’s stale review via 7a784b5 August 30, 2019 15:51

clarkdo reviewed Aug 30, 2019

View reviewed changes

fix: nest blocks

a064f70

clarkdo reviewed Aug 30, 2019

View reviewed changes

packages/utils/src/route.js Outdated Show resolved Hide resolved

manniL and others added 3 commits August 31, 2019 18:35

Update packages/utils/src/route.js

bd83c02

Co-Authored-By: Xin Du (Clark) <[email protected]>

refactor: trailingSlashes to trailingSlash

1c8c951

refactor: remove default value for trailingSlash

e79efe6

clarkdo approved these changes Aug 31, 2019

View reviewed changes

Merge branch 'dev' into feat/trailing-slash-settings

1125ba4

pi0 approved these changes Sep 1, 2019

View reviewed changes

pi0 merged commit 7c90310 into dev Sep 1, 2019

pi0 deleted the feat/trailing-slash-settings branch September 1, 2019 14:37

This was referenced Sep 1, 2019

DX utility for trailing slashes #6339

Closed

fix(generator): remove trailing slash when no subFolders are used #6362

Merged

pi0 mentioned this pull request Sep 26, 2019

v2.10.0 #6468

Merged

This was referenced Oct 3, 2019

router.trailingSlash: false "This page could not be found" #6501

Closed

en: document trailingSlash nuxt/docs#1618

Merged

This was referenced Nov 12, 2019

[Snyk] Upgrade nuxt from 2.6.3 to 2.10.2 ajesse11x/webgems#1

Open

[Snyk] Upgrade nuxt from 2.9.1 to 2.10.2 Matthelonianxl/nuxtjs#1

Open

snyk-bot mentioned this pull request Nov 25, 2019

[Snyk] Upgrade nuxt from 1.0.0-rc11 to 2.10.2 ajesse11x/css-houdini.rocks#8

Open

snyk-bot mentioned this pull request Dec 12, 2019

[Snyk] Upgrade nuxt from 2.10.0 to 2.10.2 Asthriona/Ashtriona-Blog#2

Merged

pi0 mentioned this pull request Mar 17, 2020

fix: prepend router.base when sending page track events nuxt-community/gtm-module#20

Merged

pi0 removed the WIP label Mar 17, 2020

This was referenced Apr 15, 2020

[Snyk] Upgrade nuxt from 2.5.1 to 2.12.0 mc-ireiser/nuxt-leaflet#4

Open

[Snyk] Upgrade nuxt from 2.8.1 to 2.12.0 mc-ireiser/termodaq_app#6

Open

armspkt mentioned this pull request Jul 14, 2020

Nextjs should have more options for trailing slash. vercel/next.js#15148

Closed

manniL mentioned this pull request Jan 19, 2023

Support for unified trailing slash handling #15462

Open

4 tasks

danielroe added the 2.x label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(builder, router): enforce uniform trailing slash handling #6331

feat(builder, router): enforce uniform trailing slash handling #6331

manniL commented Aug 29, 2019 •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

galvez left a comment

pimlie commented Aug 29, 2019

atinux commented Aug 29, 2019

manniL commented Aug 29, 2019 •

edited

Loading

manniL commented Aug 29, 2019 •

edited

Loading

pimlie commented Aug 30, 2019

pi0 left a comment

pi0 Aug 30, 2019

clarkdo Aug 30, 2019

pi0 Aug 30, 2019

manniL Aug 30, 2019

pi0 Sep 1, 2019

clarkdo Aug 30, 2019

manniL commented Aug 30, 2019

pimlie commented Aug 30, 2019

manniL commented Aug 31, 2019

pi0 commented Sep 1, 2019

feat(builder, router): enforce uniform trailing slash handling #6331

feat(builder, router): enforce uniform trailing slash handling #6331

Conversation

manniL commented Aug 29, 2019 • edited Loading

Types of changes

Description

TODO

Checklist:

codecov-io commented Aug 29, 2019 • edited Loading

Codecov Report

galvez left a comment

Choose a reason for hiding this comment

pimlie commented Aug 29, 2019

atinux commented Aug 29, 2019

manniL commented Aug 29, 2019 • edited Loading

manniL commented Aug 29, 2019 • edited Loading

pimlie commented Aug 30, 2019

pi0 left a comment

Choose a reason for hiding this comment

pi0 Aug 30, 2019

Choose a reason for hiding this comment

clarkdo Aug 30, 2019

Choose a reason for hiding this comment

pi0 Aug 30, 2019

Choose a reason for hiding this comment

manniL Aug 30, 2019

Choose a reason for hiding this comment

pi0 Sep 1, 2019

Choose a reason for hiding this comment

clarkdo Aug 30, 2019

Choose a reason for hiding this comment

manniL commented Aug 30, 2019

pimlie commented Aug 30, 2019

manniL commented Aug 31, 2019

pi0 commented Sep 1, 2019

manniL commented Aug 29, 2019 •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

manniL commented Aug 29, 2019 •

edited

Loading

manniL commented Aug 29, 2019 •

edited

Loading