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: sanitize some slugs that are React router dynamic routes #6510

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

Fix #5418. Not sure if this should be breaking change, since these paths probably can't be resolved previously anyways...

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Unit tests + dogfooding

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Jan 30, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 30, 2022
@netlify
Copy link

netlify bot commented Jan 30, 2022

✔️ [V2]

🔨 Explore the source changes: 40661d0

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

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

@github-actions
Copy link

github-actions bot commented Jan 30, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 72
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

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

@github-actions
Copy link

github-actions bot commented Jan 30, 2022

Size Change: +3.37 kB (0%)

Total Size: 759 kB

Filename Size Change
website/.docusaurus/globalData.json 47.9 kB +953 B (+2%)
website/build/assets/js/main.********.js 576 kB +2.41 kB (0%)
ℹ️ View Unchanged
Filename Size
website/build/assets/css/styles.********.css 105 kB
website/build/index.html 30.1 kB

compressed-size-action

@netlify
Copy link

netlify bot commented Jan 30, 2022

✔️ [V2]

🔨 Explore the source changes: f747849

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

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

@slorber
Copy link
Collaborator

slorber commented Feb 2, 2022

I have one concern with such change, because we should probably have distinct logic for 2 cases:

  • slug inferred from the FS structure: yes we want to failsafe and sanitize those to compute a good default slug
  • slug explicitly provided by the user: we should rather warn the user that some slugs are wrong and are sanitized

Are (parens) really illegal? Isn't there a way to escape special chars?

Afaik ReactRouter is using https://github.com/pillarjs/path-to-regexp

I'd like to understand how other SSGs handle those cases before taking a decision on what is best for Docusaurus

@Josh-Cena
Copy link
Collaborator Author

Are (parens) really illegal? Isn't there a way to escape special chars?

They are not illegal slugs by the URL spec, but React router interprets them as a regex capturing group. So if you have a page with slug /doc(1), it's actually served at /doc1

@slorber
Copy link
Collaborator

slorber commented Feb 2, 2022

So if you have a page with slug /doc(1), it's actually served at /doc1

But is it the case with Gatsby / Next.js / Remix too? that's worth double-checking and try to conform to what they do, particularly if someday we want to use another SSG as the underlying framework behind Docusaruus

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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File / Folder Names that contain parentheses do not resolve
3 participants