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

server: better way to handle HTTP redirects #757

Closed
shcheklein opened this issue Oct 29, 2019 · 22 comments · Fixed by #943
Closed

server: better way to handle HTTP redirects #757

shcheklein opened this issue Oct 29, 2019 · 22 comments · Fixed by #943
Labels
A: website Area: website p1-important Active priorities to deal within next sprints type: enhancement Something is not clear, small updates, improvement suggestions

Comments

@shcheklein
Copy link
Member

shcheklein commented Oct 29, 2019

Every time we move a page to a new URL we need to create a proper redirect.

The only way to this now is to create a 4-5 lines entry in our custom server.js. This is def. not scalable. We need to find an easier way to handle this.

The first solution that comes to my mind is a separate file with a list of simple expressions, may be converter functions.

@shcheklein shcheklein added A: docs Area: user documentation (gatsby-theme-iterative) duplicate This issue or pull request already exists. website: eng-doc DEPRECATED JS engine for /doc 🐛 type: bug Something isn't working. p0-critical Affects users in a bad way at the moment p1-important Active priorities to deal within next sprints and removed duplicate This issue or pull request already exists. p0-critical Affects users in a bad way at the moment labels Oct 29, 2019
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Oct 29, 2019

separate file with a list of simple expressions...

And maybe add a note with the date the redirect was added (can also be checked with GitHub Blame but it's not always straightforward) for periodic cleaning up of this list.

Another option is to use DNS for this, not the app.

@jorgeorpinel jorgeorpinel added status: research Writing concrete steps for the issue and removed 🐛 type: bug Something isn't working. labels Oct 29, 2019
@Suor
Copy link
Contributor

Suor commented Nov 1, 2019

@jorgeorpinel I don't think those should be cleaned up unless they clash with something.

Another thing - do not forget to use HTTP code 301 for moved things, not 302.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Nov 4, 2019

Actually @Suor we are having some trouble with some 301 "permanent" redirects that we now want to change again... They're cached by the browser and can't be changed again by us until that cache expires a long time later. 302 Found may be better for us since we are not so sure they are so permanent. See #768 (comment)

@shcheklein
Copy link
Member Author

I think they are still 301s @jorgeorpinel . Even though we change the end location we don't change the fact of the redirect. So, unless we don't find a good solution for caching, I would keep it 301. But I haven't googled best practices around dealing with cache yet.

@shcheklein

This comment has been minimized.

@shcheklein

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Nov 4, 2019

Well, they both have an impact. 301 is better in general but we need to be sure the redirect is actually permanent and since we're in an active development phase... I think we should not use 301s as much. Many of our redirects are temporary, which is what 302 is for. As for SEO effects:

302 redirect informs search engines to continue indexing old pages. It may impact importantly your SEO results. When you are optimizing the new page URL, make certain to be vigilant about not having duplicate content on both old and new pages at a similar time. The temporary redirect is very useful when you bring your website visitors back to the old page after some time.

https://www.firstrankseoservices.com/blog/effects-of-301-and-302-redirect-on-seo/

@shcheklein
Copy link
Member Author

Almost all our redirects are 301s in nature. We do not plan to bring your website visitors back to the old page after some time.. The only thing that is potentially not permanent is the URL we redirect to (not the fact of redirect). And probably the right way to handle this actually is to keep old 301s and add new ones (making a chain of 301s effectively).

Either we do that, or we can try no-cache as a hack.

Looks like 302s is not a good option in terms of SEO.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Nov 4, 2019

p.s we don't HAVE to use 302 Found, there's also 303 See Other, and 307 Temporary Redirect.

https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Nov 5, 2019

Full list of current redirects in server.js (from #768 (comment)) and their motivation:

  • Enforce https protocol and remove www from host - SEO
  • man.dvc.org/{cmd} -> dvc.org/doc/command-reference/{cmd}- Route design
    • replace - for / in {cmd} except for /get-url, /import-url - Backward compatibility
  • {code/data/remote}.dvc.org -> corresponding S3 bucket- Route design
  • path /(deb|rpm) -> corresponding S3 bucket- Route design
  • path /(help|chat) -> Discord chat invite- Route design
  • path /doc/commands-reference... -> /doc/command-reference... - Backward compatibility
  • path /doc/tutorial/... -> /doc/tutorials/deep/... - Backward compatibility
  • path /doc/tutorial -> /doc/tutorials - Backward compatibility
  • path /doc/use-cases/data-and-model-files-versioning -> /doc/use-cases/versioning-data-and-model-files - Backward compatibility
  • path /doc*/{item} -> /doc/{item} - Backward compatibility (also removes trailing /)

@jorgeorpinel
Copy link
Contributor

p.p.s. also there's a new 308 Permanent Redirect code in HTTP 1.1 to replace 301, and is now supposedly supported by most major browsers that have significant market share. https://tools.ietf.org/html/rfc7538#page-3

@shcheklein

This comment has been minimized.

@iAdramelk iAdramelk mentioned this issue Dec 4, 2019
@shcheklein shcheklein changed the title create a better way to handle redirects and 404s if needed create a better way to handle redirects Dec 17, 2019
@jorgeorpinel jorgeorpinel changed the title create a better way to handle redirects engine: create a better way to handle server.js redirects Dec 29, 2019
@jorgeorpinel jorgeorpinel changed the title engine: create a better way to handle server.js redirects engine: better way to handle server.js redirects Dec 29, 2019
@jorgeorpinel

This comment has been minimized.

@shcheklein

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel changed the title engine: better way to handle server.js redirects engine: better way to handle HTTP redirects Jan 4, 2020
@jorgeorpinel

This comment has been minimized.

@shcheklein

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 4, 2020

OK, well, as Next.js doesn't yet feature HTTP redirects the only way to do it is with a custom server script like we already have (/server.js) (which disables automatic static optimization unfortunately).

What I think we can do to improve this is to extract all the redirect logic to a separate module that loads the regex and targets from a JSON in order, and tests them all. That way we only ever change the JSON file in the future, which is kind of configuration instead of code (it could even live outside the Git repo).

@shcheklein
Copy link
Member Author

extract all the redirect logic to a separate module that loads the regex and targets from a JSON in order, and tests them all

yes! that was the idea of this ticket

@jorgeorpinel
Copy link
Contributor

OK got it, great. Actually though, yet another option I'd like to leave here for the record is to write and deploy a completely separate proxy server that exclusively does redirects, and that fetches our regular node app when there's no special redirect.

I'd still vote for the separate module at this point.

@jorgeorpinel jorgeorpinel changed the title engine: better way to handle HTTP redirects server: better way to handle HTTP redirects Jan 5, 2020
@jorgeorpinel
Copy link
Contributor

p.s we don't HAVE to use 302 Found, there's also 303 See Other, and 307 Temporary Redirect.

On this, I vote for 303 See Other as discussed in #875 (review).

@shcheklein
Copy link
Member Author

yep. 303 should be fine, not ideal as well but fine.

@jorgeorpinel jorgeorpinel added A: website Area: website type: enhancement Something is not clear, small updates, improvement suggestions and removed A: docs Area: user documentation (gatsby-theme-iterative) website: eng-doc DEPRECATED JS engine for /doc status: research Writing concrete steps for the issue labels Jan 19, 2020
fabiosantoscode added a commit to fabiosantoscode/dvc.org that referenced this issue Jan 23, 2020
fabiosantoscode added a commit to fabiosantoscode/dvc.org that referenced this issue Jan 24, 2020
fabiosantoscode added a commit to fabiosantoscode/dvc.org that referenced this issue Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: website Area: website p1-important Active priorities to deal within next sprints type: enhancement Something is not clear, small updates, improvement suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants