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.

Sign up for GitHub

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

Jump to bottom

Fix & refactor server routing + add tests #799

Merged
merged 11 commits into from
Jun 29, 2018
Merged

Fix & refactor server routing + add tests #799

merged 11 commits into from
Jun 29, 2018

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Jun 24, 2018 •
edited
Loading

Motivation

Changes

  • Fix sitemap.xml routing to be properly on it's correct place.
    Example: when baseUrl is /jest/, it should only be accessible at /jest/sitemap.xml and not at /sitemap.xml
  • Fix blog/atom.xml and blog/feed.xml routing to be properly in it's correct place
  • Fix wrong logic/ bug where blog/test.xml or any blog/*.xml will send blog/feed.xml
  • Fix wrong logic at blog routing in which if file does not exist, we properly exit (not send wrong file) & use next middleware function
  • Fix page routing regex, previously all .html files will hit this path, we should ignore it if it's docs or blog pages. 100% more efficient
  • Add a lot of tests with jest to catch regression & routing regex are refactored to lib/core/routing.js so that our routing is still true in v2
  • Refactor extension-less url routing + add test
  • Fix Extension-less url with dot after last slash is not accessible #796 by adding dot routing (special case like http://localhost:3000/blog/2018/05/27/1.13.0 + test

Before
before2

After
working

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Test in development, everything work as per normal
working

@endiliey endiliey added ✅ Test Plan better engineering Not a bug or feature request status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Jun 24, 2018
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 24, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 24, 2018 •
edited
Loading

Deploy preview for docusaurus-preview ready!

Built with commit cdc1c1e

https://deploy-preview-799--docusaurus-preview.netlify.com

@endiliey endiliey requested a review from yangshun June 24, 2018 10:44
@endiliey endiliey removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Jun 24, 2018
@endiliey endiliey requested a review from JoelMarcey June 24, 2018 16:38
@endiliey
Copy link
Contributor Author

Just realized that we have a lot of bugs & inefficient regex for our server routing. 😂
Please take your time to review this @yangshun @JoelMarcey

JoelMarcey
JoelMarcey approved these changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

This looks good to me, given that you added appropriate tests that look to pass. I like the continued separation of services depending on the context as well.

@JoelMarcey
Copy link
Contributor

I don't think this PR fixes it since just deals with the server and not generate.js, but we have a slight sitemap problem.

https://docusaurus.io/sitemap.xml

It contains - https://docusaurus.io/2017/12/14/introducing-docusaurus and https://docusaurus.io/2018/04/30/How-I-Converted-Profilo-To-Docusaurus

But that is the wrong URL - it should be https://docusaurus.io/blog/2017/12/14/introducing-docusaurus and https://docusaurus.io/blog/2018/04/30/How-I-Converted-Profilo-To-Docusaurus

Even shows on Google Search Console.

screenshot 2018-06-24 21 12 51

@endiliey endiliey mentioned this pull request Jun 25, 2018
Merged
@endiliey
Copy link
Contributor Author

@JoelMarcey #801 should fix it 😂

yangshun
yangshun approved these changes Jun 29, 2018
View reviewed changes
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks @endiliey! Just a couple of nits 👍

lib/server/server.js Outdated
.toString()
.split('blog/')[1]
.toLowerCase();
if (file == 'atom.xml') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - ===

lib/server/server.js Outdated
res.send(feed('atom'));
return;
} else if (file == 'feed.xml') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - ===

@endiliey endiliey merged commit e9eef39 into facebook:master Jun 29, 2018
@endiliey endiliey deleted the routing branch June 29, 2018 14:36
@endiliey endiliey mentioned this pull request Jul 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangshun yangshun yangshun approved these changes

@JoelMarcey JoelMarcey JoelMarcey approved these changes

Assignees
No one assigned
Labels
better engineering Not a bug or feature request CLA Signed Signed Facebook CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@endiliey @docusaurus-bot @JoelMarcey @yangshun @facebook-github-bot