Skip to content
This repository has been archived by the owner on Aug 17, 2024. It is now read-only.

Fix Trailing Slash Issue #254

Closed
wants to merge 1 commit into from
Closed

Conversation

ChrisOwen101
Copy link
Contributor

What does this change?

Fixes #121

Description

This fix is based on the recommendation from one of the Devs of Docusaurus. You can find it here

facebook/docusaurus#3372 (comment)

The code itself is based on a PR to the React Native website made by the same developer.

https://github.com/facebook/react-native-website/pull/2297/files#diff-4a18e8e52238979c642c1e995682a31bf24a19034400130511ac57b354c94297R1

I've tested with the two usecases in the Issue raised by @illicitonion and they both seem to be working well.

Who needs to know about this?

@illicitonion

@ChrisOwen101 ChrisOwen101 added the bug Something isn't working label May 13, 2021
Copy link
Contributor

@40thieves 40thieves left a comment

Choose a reason for hiding this comment

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

This does seem like it would solve the problem 👍🏻

But tbh I found the code a bit hard to follow (esp. without context of the Docusaurus generated structure). So I created a branch which renames some of the vars and adds a bunch of comments: fix_trailing_slash_issue...comment_fix_trailing_slash_issue. Happy to merge this in if you think it's worth it.

Additionally, what do you think about renaming the files/set up code a bit so that it's clearer what the purpose of this is?

module.exports = function () {
console.log("site plugin");
return {
plugin: "site-plugin",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could change this to be a bit more specific? I feel like it might trip us up in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the file name too.

@illicitonion
Copy link
Member

illicitonion commented May 13, 2021

I tried this out locally and I don't think it's working for me... I did an npm run build and npm run serve (because plugins only get applied for optimised builds, not for npm run start).

For instance, on this page: http://localhost:3000/git/cli/lesson
If I click the coursework link, it takes me to http://localhost:3000/git/cli/homework which works

But if I go to this page: http://localhost:3000/git/cli/lesson/
And click the same link, it takes me to http://localhost:3000/git/cli/lesson/homework which doesn't work.

I added some logging for the file copies, and it told me:
Copying /Users/dwh/src/github.com/CodeYourFuture/syllabus/build/git/cli/homework/index.html to /Users/dwh/src/github.com/CodeYourFuture/syllabus/build/git/cli/homework.html

This makes sense it's copying children into their parents, but it's not copying siblings of parents into children - it means that http://localhost:3000/git/cli/homework.html and http://localhost:3000/git/cli/homework/index.html both exist, but it doesn't change the fact that /lesson/ doesn't have homework-related children.


I looked at a few ways to maybe fix this which didn't involve replicating file trees, because having to replicate parents into children doesn't seem scalable... Some things I tried:

  • Parsing the generated HTML, rewriting href attributes, and writing it out again. This didn't work because there's a webpack-based content loader which replaces the contents of the HTML files with assets dynamically.
  • I was able to re-configure how the docusaurus serve command behaves to strip trailing slashes by reconfiguring how it invokes serve-handler to add trailingSlash: false to its config, but that doesn't help with our GitHub Pages hosting.
  • I tried out this workaround which removes trailing slashes from navigated-to pages in JS. This one seems to work ok... I've made Remove trailing slashes when on pages. #258 so we can see how they compare :)

illicitonion added a commit to illicitonion/syllabus that referenced this pull request May 13, 2021
@ChrisOwen101
Copy link
Contributor Author

Closed in favour of #258

illicitonion added a commit that referenced this pull request May 15, 2021
@SallyMcGrath SallyMcGrath deleted the fix_trailing_slash_issue branch November 26, 2021 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
3 participants