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

fix(generator): avoid duplicate slashes for routes ending with hash #7776

Merged
merged 4 commits into from
Jul 27, 2020

Conversation

farnabaz
Copy link
Member

@farnabaz farnabaz commented Jul 26, 2020

Using links like /foo/#bar cause error in generating static pages. This happens because crawler adds a trailing slash to every route. We need to make sure the is no duplicate slashes in route.

close #7772

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

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-commenter
Copy link

codecov-commenter commented Jul 26, 2020

Codecov Report

Merging #7776 into dev will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #7776      +/-   ##
==========================================
+ Coverage   68.83%   68.85%   +0.02%     
==========================================
  Files          90       90              
  Lines        3821     3824       +3     
  Branches     1034     1034              
==========================================
+ Hits         2630     2633       +3     
  Misses        968      968              
  Partials      223      223              
Flag Coverage Δ
#unittests 68.85% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/generator/src/generator.js 86.22% <ø> (ø)
packages/webpack/src/utils/postcss.js 84.52% <0.00%> (ø)
packages/utils/src/route.js 96.73% <0.00%> (+0.06%) ⬆️

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 45f204c...44af4f0. Read the comment docs.

pi0
pi0 previously approved these changes Jul 26, 2020
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You rock @farnabaz 👍

@pi0 pi0 changed the title fix: remove duplicate slashes from crawled routes fix(generator): avoid duplicate slashes for routes ending with hash Jul 26, 2020
@pi0
Copy link
Member

pi0 commented Jul 26, 2020

Updating full-static fixture with trailingSlash enabled, it seems we have other problems with it as well:

ERROR Error generating route "/pagination/1/2/": This page could not be found
ERROR Error generating route "/pagination/1/0/": This page could not be found

Also client-side routing is broken. Maybe we should make a warning or force disable trailing slash for static target to avoid this issues?

@farnabaz
Copy link
Member Author

farnabaz commented Jul 26, 2020

Maybe we should make a warning or force disable trailing slash for static target to avoid this issues?

This isn't about static target, Using trailingSlash and relative urls at the same time will cause the problem. I believe we should warn users about the behavior of relative urls with and without tailing slash.

If you look at full-static/pages/pagination/_i.vue you will see that links are relative and with trailingSlash enabled it always generates wrong url. It is safer to use absolute paths when trailingSlash is enable.

<NLink v-if="i>0" :to="`./${i-1}`">
      Previous
</NLink>

@pi0
Copy link
Member

pi0 commented Jul 27, 2020

I believe we should warn users about the behavior of relative urls with and without tailing slash.

Agree. Anyway, this is a bug :D Was talking with @Atinux we probably going to remove trailingSlash support for nuxt3 since it is adding more and more edge-cases.

@pi0 pi0 merged commit 298c3e3 into dev Jul 27, 2020
@pi0 pi0 deleted the crawler-duplicate-slash branch July 27, 2020 09:01
@pi0 pi0 mentioned this pull request Jul 27, 2020
@papertokyo
Copy link

@pio what will removing trailingSlash support mean for Netlify users? We're currently using that config setting because Netlify rewrites urls with a slash and it messes up SEO/loading performance with superfluous redirects otherwise. (At least, that's my understanding.)

@mornir
Copy link

mornir commented Jul 29, 2020

Yeah, I'm also using the trailing slash because of Netlify:
https://dev.to/mornir/nuxt-netlify-and-the-trailing-slash-3gge

@papertokyo it's possible to disable the redirection: gatsbyjs/gatsby#15317 (comment), but it's not possible to have Netlify automatically redirect routes that end with a trailing slash to their non trailing slash equivalent (but the opposite is possible: non trailing slash --> trailing slash).

@TorbjornHoltmon
Copy link

A very annoying limitation from netlify...
I have had to go all-in on trailing slashes too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

export error with hash link, baseurl and trailingSlash
7 participants