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

feat(v2): add trailingSlash config option #4908

Merged
merged 15 commits into from
Jun 9, 2021
Merged

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 4, 2021

Motivation

Fixes #3372

After analyzing the behavior of multiple static hosting providers (see https://github.com/slorber/trailing-slash-guide), I think we should have a trailingSlash option (like Next.js has)

This is a risky change that can have deep SEO impacts for some sites, so we are keeping a retrocompatible behavior by default.

It would allow 3 values:

  • true: force a trailing slash to SPA routes + links, output /myPath/index.html
  • false: remove trailing slashes on SPA routes + links, output /myPath.html
  • undefined: retrocompatible behavior: leave routes unchanged (often no trailing slash), output /myPath/index.html

If the new behaviors are successful, we should add them to the init template, and deprecate the undefined legacy behavior with a Joi warning, telling the users to be extra careful about the potential SEO impacts.

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

Tests + deploy previews


I'm implementing a new {trailingSlash: boolean | undefined} config.

  • undefined: it keeps the existing retro-compatible behavior (paths not modified), and output /path/index.html
  • false: remove a potential trailing slash on routes, links, canonical URLs, and output /path.html
  • true: force a trailing slash on routes, links, canonical URLs, and output /path/index.html

Refer to https://github.com/slorber/trailing-slash-guide for how the files will be served by your host.

I've created 3 deployments to test this feature:

Note: using true/false instead of undefined allows to use Netlify with pretty URLs on without having any annoying redirection, but it's not the case for the undefined behavior that will keep redirecting with pretty URLs on (default behavior). Had to disable this.

PLEASE: help me review those deployments and let me know if you see any unwanted side-effects: now is a better time to complain than after merging the PR :)

Note: this feature is generic and in core, to avoid the need to have each plugin implement custom trailingSlash logic. In some cases it might require some plugin tweaks but it should work fine in a generic way for plugin authors in most cases.

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jun 4, 2021
@slorber slorber requested a review from lex111 as a code owner June 4, 2021 18:05
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 4, 2021
@slorber slorber marked this pull request as draft June 4, 2021 18:07
@netlify
Copy link

netlify bot commented Jun 4, 2021

✔️ [V1]

🔨 Explore the source changes: c17235f

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-1/deploys/60c0fe24933cf8000703b391

😎 Browse the preview: https://deploy-preview-4908--docusaurus-1.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

Size Change: 0 B

Total Size: 620 kB

ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 87.7 kB 0 B
website/build/assets/js/main.********.js 443 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 62.1 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 27 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Jun 8, 2021

✔️ [V2]

🔨 Explore the source changes: c17235f

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60c0fe2470bcda0008648986

😎 Browse the preview: https://deploy-preview-4908--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 58
🟢 Accessibility 97
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4908--docusaurus-2.netlify.app/

@slorber slorber marked this pull request as ready for review June 8, 2021 18:02
@thehappybug
Copy link

@slorber The docs page is missing the navigation on the left. I can't tell if this is intentional or a bug introduced in this PR.

See page https://docusaurus-trailing-slash-false.netlify.app/docs.

This has the right behavior for me based on my requirements in #3372 (comment).

I see that the URLs in the pages, in the canonical meta tag, the sitemap, and the browser bar are all matching and contain trailing slashes. And Netlify's Pretty URL redirects users correctly.

I will now wait for the canary build to test this on my site.

@slorber
Copy link
Collaborator Author

slorber commented Jun 9, 2021

@slorber The docs page is missing the navigation on the left. I can't tell if this is intentional or a bug introduced in this PR.

Thanks, definitively not intentional but understand why it happens, due to using slug: "/". Other docs work fine see https://docusaurus-trailing-slash-false.netlify.app/docs/installation
Will fix this asap.

@thehappybug
Copy link

I wasn't able to get my site working with the current canary release. I get build errors and I'm not sure what the cause is.

Yarn install output
PS D:\Repositories\Documentation\apimatic-docs> yarn upgrade @docusaurus/core@canary  @docusaurus/preset-classic@canary
yarn upgrade v1.22.10
[1/4] Resolving packages...
[2/4] Fetching packages...
info fsevents@2.3.2: The platform "win32" is incompatible with this module.
info "fsevents@2.3.2" is an optional dependency and failed compatibility check. Excluding it from installation.
info fsevents@1.2.13: The platform "win32" is incompatible with this module.
info "fsevents@1.2.13" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning "@docusaurus/preset-classic > @docusaurus/theme-search-algolia > @docsearch/react@3.0.0-alpha.36" has unmet peer dependency "@types/react@>= 16.8.0 < 18.0.0".
y "@algolia/client-search@^4.5.1".
[4/4] Rebuilding all packages...
success Saved lockfile.
success Saved 8 new dependencies.
info Direct dependencies
└─ @docusaurus/preset-classic@2.0.0-beta.aff848e87
info All dependencies
├─ @docusaurus/cssnano-preset@2.0.0-beta.aff848e87
├─ @docusaurus/plugin-debug@2.0.0-beta.aff848e87
├─ @docusaurus/plugin-google-analytics@2.0.0-beta.aff848e87
├─ @docusaurus/plugin-google-gtag@2.0.0-beta.aff848e87
├─ @docusaurus/plugin-sitemap@2.0.0-beta.aff848e87
├─ @docusaurus/preset-classic@2.0.0-beta.aff848e87
├─ @docusaurus/theme-classic@2.0.0-beta.aff848e87
└─ @docusaurus/theme-search-algolia@2.0.0-beta.aff848e87
Done in 61.44s.
Build output (has error)
Unable to build website for locale en.
RangeError: Invalid time value
    at D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\plugin-content-blog\lib\blogUtils.js:118:12
    at async Promise.all (index 16)
    at async Object.generateBlogPosts (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\plugin-content-blog\lib\blogUtils.js:84:5)
    at async Object.loadContent (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\plugin-content-blog\lib\index.js:59:25)
    at async D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\server\plugins\index.js:54:46
    at async Promise.all (index 6)
    at async Object.loadPlugins (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\server\plugins\index.js:53:34)
    at async Object.load (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\server\index.js:102:82)
    at async buildLocale (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\commands\build.js:80:19)
    at async tryToBuildLocale (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\commands\build.js:32:20)
    at async Object.mapAsyncSequencial (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\utils\lib\index.js:328:24)
    at async build (D:\Repositories\Documentation\apimatic-docs\node_modules\@docusaurus\core\lib\commands\build.js:68:25)
error Command failed with exit code 1.

Let me know if this is something that will go away with the next beta release. If this feels like a bug, let me know how I can debug it to help avoid this going to release.

@slorber
Copy link
Collaborator Author

slorber commented Jun 15, 2021

@thehappybug this does not look related to trailing slashes. From which version are you upgrading? May be related to refactor in #4759

It looks like we can't compute a proper date for some of your blog posts using this code:

      date = date ?? (await fs.stat(source)).birthtime;
      const formattedDate = new Intl.DateTimeFormat(i18n.currentLocale, {
        day: 'numeric',
        month: 'long',
        year: 'numeric',
      }).format(date);

Can you give me an idea of your blog post filenames + their frontmatter please? Do you see any post with a potentially wrong date filename pattern or frontmatter?

As this is unrelated, I'd appreciate if you open a new issue instead of commenting this PR

@Labirintami
Copy link

Hello, is this update included in the latest public builds?

@thehappybug
Copy link

@Labirintami It is only available on canary right now.

See: #3372 (comment)

@slorber
Copy link
Collaborator Author

slorber commented Jun 17, 2021

will be in beta.1, probably tomorrow

I've written how to use a Canary release here: https://docusaurus.io/community/canary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix trailing slash issues (depend on hosting provider)
4 participants