server: refactor redirects so they're more declarative

[fabiosantoscode]

This places redirects in a separate file and attempts to simplify them.

I've identified two kinds of redirects, a redirect based on the host, and a redirect based on pathname. I've used classes but I'm not married to that approach.

I've added cache-control: no-cache to all redirects and they're all 303's, but this approach can be extended to include some options specific to each redirect as an options object in the constructor.

Fixes #757

---

Disregard the recommendations below if you use Edit on GitHub button to improve the docs in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This enables GitHub to link the PR to the corresponding bug and close it automatically when PR is merged.

Thank you for the contribution - we'll try to review and merge it as soon as possible. 🙏

[fabiosantoscode]

Looks like an unrelated lint issue popped up in a file I didn't change.

[shcheklein]

@fabiosantoscode okay, this is a good ticket, but we should handle it in a different way. Let's discuss this on Discord.

[shcheklein]

@fabiosantoscode check the comment re the 301 vs 303, let's try to wrap it as a worker for the Cloudflare. Move redirects.js into redirects.json and try to make it way more declarative.

[fabiosantoscode]

@shcheklein do you mean that you want me to make this a worker right now? I couldn't find any existing cloudflare workers on this repository.

[fabiosantoscode]

@shcheklein I've addressed the part of the feedback I'm sure about, by making some redirects permanent and others not. I've also changed the file to JSON and used an approach that will be familiar to anyone who knows how ''.replace works.

• match will indicate what to match against, currently only url for the whole URL or pathname for just the pathname
• regex is the regexp. The groups are important here.
• replace is passed verbatim to the second argument of ''.replace. Here you can use $1, $2, etc to refer to strings captured in the regex groups.
• permanent causes a permanent redirect. At first I used 308 but I figured support for this would be worse than 301, and after looking at the RFCs it looks like they don't have any semantic difference, and no behaviour difference except for POST requests, which are not a thing on this website.

Please let me know if you'd like me to create a service worker which could be used in the browser or cloudflare, or if the changes I made were only in anticipation of moving to cloudflare workers.

Cheers! ⛄

[fabiosantoscode]

Is it me or codeclimate is being extremely picky?

[shcheklein]

@fabiosantoscode yes, pay reasonable attention to it, no need to fix all complaints :)

[shcheklein]

Good stuff! Next steps here:

1. this json is too bloated, hard to read, manage, follow. Let's do an array of "expression redirect code". We can figure out from the expression if it's url or path, we can specify explicitly the code (and by default consider it 301).
2. we need tests. Not that we have a wrapper we can write some unit tests for this and check carefully. Also tests should be able to detect loops.

[fabiosantoscode]

@shcheklein So an array of space-separated strings then?

The tests will be cool to write, I'll get on it.

[fabiosantoscode]

@shcheklein I've used a bunch of spaces to align the targets and make the JSON file easier to read, even if it's wider.

I've had to create a itRedirects "custom it function" to try and make the tests concise.

[fabiosantoscode]

I mean, I didn't have to but it seemed nicer.

[shcheklein]

awesome! just a few minor comments/requests for improvement, and let's wait for @jorgeorpinel to have a second look.

[shcheklein]

looks awesome! let's wait for @jorgeorpinel to take a look and merge and test after that. I wonder if there is also a service somewhere to setup that will be pinging website for predefined URLs and check if they behave properly?

[fabiosantoscode]

@shcheklein this sounds like something pingdom can do, but I'm not sure we can check where the redirect goes to.

We can also use circleCI to run periodic tests against production. We can reuse the redirects tests I added, but add an environmental variable that tells them to hit production. We can add more tests like this later.

Another thing that might work is google cloud platform uptime checks. You can poll a certain URL (like man.dvc.org/add) and make sure the response content contains or matches a certain string. However this is only feasible if we add a message explaining the redirect containing a redirect link target as the redirect response body. Additionally, another similar check can check for the uptime of the blog itself. And these uptime checks can ping people through e-mail, SMS and other media.

[shcheklein]

merging this, if we hit any issue will be solving as we go. Takes too long otherwise.

[jorgeorpinel]

Should this live in src/utils @fabiosantoscode @shcheklein ? Thanks

[shcheklein]

yep, we can move them .. move redirects-list to the root porbably

[fabiosantoscode]

Will move and PR

[jorgeorpinel]

Thanks!

[jorgeorpinel]

My last comment here is that I personally find the space separation in the JSON file strange. A normal JSON file with an array of arrays or something like that should work as well and easier to read? Let me review the new PR and will see if this comment is still valid. I may also just push some suggested changes myself if needed. Thanks

[shcheklein]

@jorgeorpinel we had an iteration like this and I didn't like it (not sure about @fabiosantoscode :) ) - it's 1000 lines vs 10. It's way easier to read line by line. It's very regular format for redirects files - one rule a line. There should be very strong reasons to complicate this.