From ca28a4df4fa8f593fe9bfc189b92764e1344e71b Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Tue, 4 Feb 2020 13:08:14 -0600 Subject: [PATCH 1/2] server: reformat/refactor code and comments around new redirects as mentioned in https://github.com/iterative/dvc.org/pull/970#pullrequestreview-353200254 --- server.js | 28 ++++++++++++++------------ src/utils/redirects.js | 2 +- src/utils/redirects.test.js | 40 ++++++++++++++++++------------------- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/server.js b/server.js index aece05494d..e0422aa933 100644 --- a/server.js +++ b/server.js @@ -10,7 +10,8 @@ */ const { createServer } = require('http') -const { parse } = require('url') +const { parse: parseURL } = require('url') +const { parse: parseQuery } = require('querystring') const next = require('next') const { getItemByPath } = require('./src/utils/sidebar') @@ -23,23 +24,21 @@ const port = process.env.PORT || 3000 app.prepare().then(() => { createServer((req, res) => { - const parsedUrl = parse(req.url, true) - const { pathname, query } = parsedUrl + const parsedUrl = parseURL(req.url) + const { pathname, queryStr } = parsedUrl const host = req.headers.host - /* - * HTTP redirects - */ let [redirectCode, redirectLocation] = getRedirect(host, pathname, { req, dev }) - if (redirectLocation) { - // should be getting the query as a string - const { query } = parse(req.url) - if (query) { - redirectLocation += '?' + query + /* + * HTTP redirects + */ + + if (queryStr) { + redirectLocation += '?' + queryStr } res.writeHead(redirectCode, { 'Cache-control': 'no-cache', @@ -57,9 +56,12 @@ app.prepare().then(() => { } // Custom route for all docs - app.render(req, res, '/doc', query) + app.render(req, res, '/doc', parseQuery(queryStr)) } else { - // Regular Next.js handler + /* + * Regular Next.js handler + */ + handle(req, res, parsedUrl) } }).listen(port, err => { diff --git a/src/utils/redirects.js b/src/utils/redirects.js index 58b86cc110..44b119af9e 100644 --- a/src/utils/redirects.js +++ b/src/utils/redirects.js @@ -17,7 +17,7 @@ const processRedirectString = redirectString => { exports.processRedirectString = processRedirectString -// Parse redirects when starting up +// Parse redirects when starting up. redirects = redirects.map(processRedirectString) const matchRedirectList = (host, pathname) => { diff --git a/src/utils/redirects.test.js b/src/utils/redirects.test.js index c31596221d..81cb60d127 100644 --- a/src/utils/redirects.test.js +++ b/src/utils/redirects.test.js @@ -3,41 +3,41 @@ const { processRedirectString, getRedirect } = require('./redirects') describe('processRedirectString', () => { it('reads the regex, replacement and code', () => { - const { regex, replace, code } = processRedirectString('^/foo /bar 418') + const { regex, replace, code } = processRedirectString('^/foo /bar 308') expect(regex).toBeInstanceOf(RegExp) expect(regex.source).toEqual('^\\/foo') expect(replace).toEqual('/bar') - expect(code).toEqual(418) + expect(code).toEqual(308) }) - it('code defaults to 301', () => { - const { code } = processRedirectString('^/x /x') + it('defaults to 301 status code', () => { + const { code } = processRedirectString('^/x /y') expect(code).toEqual(301) }) - it('detects whether we are matching a pathname or the whole url', () => { - const { matchPathname } = processRedirectString('^/pathname /x') - expect(matchPathname).toEqual(true) - + it('detects whether matching a full URL or just the path', () => { const { matchPathname: matchPathnameFalse } = processRedirectString( '^https://example.com/foo /x' ) expect(matchPathnameFalse).toEqual(false) + + const { matchPathname } = processRedirectString('^/path /y') + expect(matchPathname).toEqual(true) }) }) describe('getRedirect', () => { - it('redirects to https while removing www', () => { - const fakeReq = { + it('redirects to https:// while removing www', () => { + const mockReq = { headers: { 'x-forwarded-proto': 'http' }, url: '/foo/bar?baz' } expect( - getRedirect('www.example.com', '/not-used', { req: fakeReq, dev: false }) + getRedirect('www.example.com', '/not-used', { req: mockReq, dev: false }) ).toEqual([301, 'https://example.com/foo/bar?baz']) }) @@ -57,18 +57,17 @@ describe('getRedirect', () => { expect(rLocation).toEqual(target) expect(rCode).toEqual(code) - // Detect redirect loops + // Detect redirect loops. const secondUrl = url.parse(addHost(rLocation)) const secondRedirect = getRedirect(secondUrl.hostname, secondUrl.pathname) expect(secondRedirect).toEqual([]) }) } - describe('host redirects', () => { - // remove the www (when already HTTPS) + describe('Subdomain-based redirects', () => { + // Remove www (when already HTTPS) itRedirects('https://www.dvc.org/foo', 'https://dvc.org/foo') - // short and sweet hosts itRedirects( 'https://man.dvc.org/', 'https://dvc.org/doc/command-reference/', @@ -94,7 +93,7 @@ describe('getRedirect', () => { ) }) - describe('redirects to s3', () => { + describe('S3 redirects', () => { itRedirects( 'https://code.dvc.org/foo/bar', 'https://s3-us-east-2.amazonaws.com/dvc-public/code/foo/bar', @@ -126,22 +125,23 @@ describe('getRedirect', () => { ) }) - describe('discord', () => { + describe('Discord', () => { itRedirects('/help', 'https://discordapp.com/invite/dvwXA2N', 303) itRedirects('/chat', 'https://discordapp.com/invite/dvwXA2N', 303) }) - describe('in-site moves', () => { + describe('Path redirects', () => { itRedirects('/docs/x', '/doc/x') itRedirects('/documentation/x', '/doc/x') - itRedirects('/doc/commands-reference/add', '/doc/command-reference/add') + itRedirects('/doc/commands-reference/foo', '/doc/command-reference/foo') + itRedirects('/doc/tutorial', '/doc/tutorials') itRedirects('/doc/tutorial/', '/doc/tutorials') - itRedirects('/doc/tutorial/subject', '/doc/tutorials/deep/subject') + itRedirects('/doc/tutorial/bar', '/doc/tutorials/deep/bar') itRedirects( '/doc/use-cases/data-and-model-files-versioning', From b6edc5f433123b3962e14d1cb74c283690f00811 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Wed, 19 Feb 2020 02:39:31 -0600 Subject: [PATCH 2/2] links: addresses #973 PR feedback per https://github.com/iterative/dvc.org/pull/973#pullrequestreview-353269240 through https://github.com/iterative/dvc.org/pull/973#pullrequestreview-353270157 --- server.js | 12 +++--------- src/utils/redirects.test.js | 16 ++++++++-------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/server.js b/server.js index e0422aa933..f411096bed 100644 --- a/server.js +++ b/server.js @@ -33,9 +33,7 @@ app.prepare().then(() => { dev }) if (redirectLocation) { - /* - * HTTP redirects - */ + // HTTP redirects if (queryStr) { redirectLocation += '?' + queryStr @@ -46,9 +44,7 @@ app.prepare().then(() => { }) res.end() } else if (/^\/doc(\/.*)?$/.test(pathname)) { - /* - * Docs Engine handler - */ + // Docs Engine handler // Force 404 response code for any inexistent /doc item. if (!getItemByPath(pathname)) { @@ -58,9 +54,7 @@ app.prepare().then(() => { // Custom route for all docs app.render(req, res, '/doc', parseQuery(queryStr)) } else { - /* - * Regular Next.js handler - */ + // Regular Next.js handler handle(req, res, parsedUrl) } diff --git a/src/utils/redirects.test.js b/src/utils/redirects.test.js index 81cb60d127..2d77809ef8 100644 --- a/src/utils/redirects.test.js +++ b/src/utils/redirects.test.js @@ -11,13 +11,13 @@ describe('processRedirectString', () => { expect(code).toEqual(308) }) - it('defaults to 301 status code', () => { + it('defaults to 301 response code', () => { const { code } = processRedirectString('^/x /y') expect(code).toEqual(301) }) - it('detects whether matching a full URL or just the path', () => { + it('detects whether redirecting a full URL or just a path', () => { const { matchPathname: matchPathnameFalse } = processRedirectString( '^https://example.com/foo /x' ) @@ -28,8 +28,8 @@ describe('processRedirectString', () => { }) }) -describe('getRedirect', () => { - it('redirects to https:// while removing www', () => { +describe('getRedirects', () => { + it('enforces HTTPS and removes www simultaneously', () => { const mockReq = { headers: { 'x-forwarded-proto': 'http' @@ -64,7 +64,7 @@ describe('getRedirect', () => { }) } - describe('Subdomain-based redirects', () => { + describe('fromSubdomains', () => { // Remove www (when already HTTPS) itRedirects('https://www.dvc.org/foo', 'https://dvc.org/foo') @@ -93,7 +93,7 @@ describe('getRedirect', () => { ) }) - describe('S3 redirects', () => { + describe('toS3', () => { itRedirects( 'https://code.dvc.org/foo/bar', 'https://s3-us-east-2.amazonaws.com/dvc-public/code/foo/bar', @@ -125,13 +125,13 @@ describe('getRedirect', () => { ) }) - describe('Discord', () => { + describe('toDiscord', () => { itRedirects('/help', 'https://discordapp.com/invite/dvwXA2N', 303) itRedirects('/chat', 'https://discordapp.com/invite/dvwXA2N', 303) }) - describe('Path redirects', () => { + describe('fromPaths', () => { itRedirects('/docs/x', '/doc/x') itRedirects('/documentation/x', '/doc/x')