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

[Feature][SEO] Allow strict and case sensitive paths #3448

Closed
wants to merge 2 commits into from

Conversation

@MitsuhaKitsune
Copy link

commented Jun 15, 2018

Actually nuxt.js have a duplicated content issue on generated routes, you can access to same page on diferent url's variations like: '/news', '/news/', '/News', '/NeWS/', etc etc...

This is bad practice for SEO and it's penalized by search engines, with this modification nuxt.js allow set strict and case sensitive routes with some options on nuxt.config.js inside router section:

  router: {
    routesOptions: {
      pathToRegexpOptions: {strict: true, sensitive: true},
      trailingSlash: true
    }
  }

Example scenario

Files:

pages/
 |-index.vue
 |-contact.vue
 |-blog/
 |   |-index.vue
 |   |-_slug.vue
 |-otherpages/
    |-index.vue
    |-one.vue
    |-two.vue

Generated routes urls (should match exactly, any diference on path didnt meet the route, except the fourth url that it's dinamic but still working only without final slash):

/
/contact
/blog/
/blog/somedinamicslug
/otherpages/
/otherpages/one
/otherpages/two

@MitsuhaKitsune MitsuhaKitsune force-pushed the MitsuhaKitsune:feature-3100 branch Jun 15, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Jun 15, 2018

Codecov Report

Merging #3448 into dev will decrease coverage by 0.17%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev    #3448      +/-   ##
=========================================
- Coverage   99.1%   98.92%   -0.18%     
=========================================
  Files         18       18              
  Lines       1112     1116       +4     
  Branches     299      301       +2     
=========================================
+ Hits        1102     1104       +2     
- Misses        10       12       +2
Impacted Files Coverage Δ
lib/common/nuxt.config.js 100% <ø> (ø) ⬆️
lib/builder/builder.js 100% <ø> (ø) ⬆️
lib/common/utils.js 98.82% <66.66%> (-1.18%) ⬇️

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 feabdca...7460b73. Read the comment docs.

lib/common/utils.js Outdated
@@ -254,7 +254,7 @@ function cleanChildrenRoutes(routes, isChild = false) {
return routes
}

exports.createRoutes = function createRoutes(files, srcDir, pagesDir) {
exports.createRoutes = function createRoutes(files, srcDir, pagesDir, strictRoutes, caseSensitive) {

This comment has been minimized.

Copy link
@clarkdo

clarkdo Jun 15, 2018

Member

Maybe extract them into a options parameter is better for future extensible.

This comment has been minimized.

Copy link
@MitsuhaKitsune
lib/common/utils.js Outdated
route.pathToRegexpOptions = { strict: strictRoutes, sensitive: caseSensitive }

// Add final trailing slash to path on index.vue files inside pages subfolders
if (strictRoutes && /-index$/.test(route.name)) {

This comment has been minimized.

Copy link
@clarkdo

clarkdo Jun 15, 2018

Member

How about adding a option for this behavior for not affecting legacy functionality.

This comment has been minimized.

Copy link
@MitsuhaKitsune

MitsuhaKitsune Jun 15, 2018

Author

Yes you are right, I add routeCreationOptions.autoTrailingSlashSubfolders for not affect legacy funcionality

@MitsuhaKitsune MitsuhaKitsune force-pushed the MitsuhaKitsune:feature-3100 branch Jun 15, 2018

lib/common/utils.js Outdated
@@ -254,7 +254,7 @@ function cleanChildrenRoutes(routes, isChild = false) {
return routes
}

exports.createRoutes = function createRoutes(files, srcDir, pagesDir) {
exports.createRoutes = function createRoutes(files, srcDir, pagesDir, creationOptions) {

This comment has been minimized.

Copy link
@clarkdo

clarkdo Jun 15, 2018

Member

Name to be options or use destructuring assignment should be more appropriate.

This comment has been minimized.

Copy link
@MitsuhaKitsune

MitsuhaKitsune Jun 16, 2018

Author

Now I use there a destructuring assignment as you recomend, but I don't know if I do it fine, it's the first time that I use it as function arguments

lib/common/options.js Outdated
@@ -308,7 +308,12 @@ Options.defaults = {
scrollBehavior: null,
parseQuery: false,
stringifyQuery: false,
fallback: false
fallback: false,
routeCreationOptions: {

This comment has been minimized.

Copy link
@clarkdo

clarkdo Jun 15, 2018

Member

How about use routesOptions and make strictRegexPaths and caseSensitivePaths be pathToRegexpOptions as same as route config .

routesOptions: {
  //{strict: {true|false}, sensitive: {true|false}}
  pathToRegexpOptions: undefined,
  trailingSlash: false
}

Another important thing is that you should develop based on dev branch which options.defaults has been moved to lib/common/options.

This comment has been minimized.

Copy link
@MitsuhaKitsune

MitsuhaKitsune Jun 16, 2018

Author

Sorry I push to 1.x because I think that dev branch it's the comming 2.x, I gona rebase the PR on dev branch with your recomendations

@MitsuhaKitsune MitsuhaKitsune force-pushed the MitsuhaKitsune:feature-3100 branch Jun 16, 2018

@MitsuhaKitsune MitsuhaKitsune changed the base branch from 1.x to dev Jun 16, 2018

@MitsuhaKitsune MitsuhaKitsune force-pushed the MitsuhaKitsune:feature-3100 branch to 607a32e Jun 16, 2018

@MitsuhaKitsune

This comment has been minimized.

Copy link
Author

commented Jun 16, 2018

@clarkdo: Rebased to dev branch and applied all requested changes, sorry for the inconveniences.

Check if I do fine on https://github.com/nuxt/nuxt.js/pull/3448/files#diff-963dc90aa712aa2f54692cc2542c65feR248, it's the first time that I use destructuring assignments as function arguments.

@clarkdo

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

@MitsuhaKitsune
Looks good for me now, can you add test cases for you changes ?

/cc @Atinux @pi0

@miljan-aleksic

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

@clarkdo, not sure why I was mentioned here :/

@Atinux

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

Hi @MitsuhaKitsune

I am not sure that adding a new option is the solution.

Actually we already talked about this before and you simply have to use the rel=canonical meta header to tell Google or any SEO crawlers how to avoid duplicate.

You can see an implementation our our documentation website: https://github.com/nuxt/nuxtjs.org/blob/master/layouts/default.vue#L26

More information about canonical: https://support.google.com/webmasters/answer/139066?hl=en

@clarkdo

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@Atinux @MitsuhaKitsune
Oops, sorry about that I didn't notice that ticket.

@miljan-aleksic
Sorry for bothering you, I referred the wrong guy.

@MitsuhaKitsune

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

Hi @Atinux

I know how work the canonical metatags, but I think isn't a good practice for this case, canonical tags are usefull (or atleast indicative for some search engines) to prevent duplicated content issues on websites, but on another scenarios, think on a online shop for example, we have the following urls:

  • /t-shirts/
  • /t-shirts/price
  • /t-shirts/alphabethical

The three url's got the same content, a t-shirts product list, but the second and third url's are used to sort the list by price or alphabethical, the order of the list don't affect to similarity algorritm, these three url's gona get flagged as duplicated content, to prevent it, we can choose one of these three to get indexed, for example /t-shirts/ and hint the crawlers to index only this url and ignore the other two.

On this example case, we need the three url's active, we don't have another way (except make dynamic and realtime filters without affect the url) so canonical meta tag help us on this case.

Explained on https://www.rocketclicks.com/client-education/canonical-tag-guide-choosing-canonical-vs-redirect/


On the example case exposed on the pull request, we have a little news blog in our nuxt, we only need /news/ url to access to it, /news, /News and /NeWS/ (have more variations, but I write only these 4 on the pull request example) only are distorting our webpage structure and make a lot of available urls to access the same page, with same content (and on difference with the shop example, with same order, 100% of similarity) without need it, canonical tags can 'patch' (atleast on better search engines) but isn't a better solution for this case, we should only offer a one url here, and the correct are /news/ on this case (explained bellow).

(I can't find now article for this example case)


Note that on previous examples, I use the final trailing slash on some url's, the reason it's for identify the url as subfolder because have 'sub-url's inside', come back to the shop example, when we click one product of the list, we have a only single product page on /t-shirts/black-male-t-shirt-nuxt-logo (it's a subpage of t-shirts), and we have a only single page to our refounds policy for example on /refounds, putting the final trailling slash or not correctly we help the search engines and users to understand better the structure of our site.

Explained on https://en.pedrodias.net/seo/trailing-slash-urls


I'm not a SEO expertise and I talking from my experience, as developer I help to website owners to improve their SEO and get better visibility on search engines reading a lot of SEO guides and asking on sites of SEO experts (a lot of time past of it, can't recover all links), it's possible that I wrong on some point, but for now all sites where I apply this things have a good SEO and don't recive any negative coment from their owners.

Note that I don't making any redirection, only I choose a unic and the best path for each route and make it strict, website users don't have affected with extra load time or redirections.

@MitsuhaKitsune

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

@clarkdo No problem, I didn't noticed that this case are reviewed on the past too, I gona wait for @Atinux decission, if he decide continue with this I upload the tests (I'm new on Node.js and I need learn how unit tests work first, I tried checking the current test, but I use npm not yarn, so I can't run the test through CLI commands on my PC).

Anyways thanks to both for this awesome project, I really love it and I gona still here to help when I can, and specially to you @clarkdo for the improvement tips and the time taked on check this, your tips are really usefull and I learn a bit from them ^^

@pi0
Copy link
Member

left a comment

  • Introducing case-sensitive paths means more inconsistencies between windows/mac and Linux (production builds)
  • Adding a trailing slash for stricter route matching would be somehow a breaking change
@MitsuhaKitsune

This comment has been minimized.

Copy link
Author

commented Jun 25, 2018

Okay, seems enought reasons for don't merge this PR, sorry for the inconveniences

@lock

This comment has been minimized.

Copy link

commented Nov 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.