From be81e9372a0794871b0f6ad3c4d09036aa71cdf9 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Tue, 4 Feb 2020 13:08:14 -0600 Subject: [PATCH 1/8] 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 d629a37ddef371b6df9dd23636bd347f9753d698 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Wed, 19 Feb 2020 02:39:31 -0600 Subject: [PATCH 2/8] 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') From 10b375654cf09d1b6c9a5bda37168950f5eb4c8c Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 12:55:05 -0600 Subject: [PATCH 3/8] server: attempr to fix null query bug per https://github.com/iterative/dvc.org/pull/1009#issuecomment-588748865 ref https://github.com/zeit/next.js/issues/7303#issuecomment-511053550 --- server.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server.js b/server.js index f411096bed..a52a96add4 100644 --- a/server.js +++ b/server.js @@ -52,7 +52,9 @@ app.prepare().then(() => { } // Custom route for all docs - app.render(req, res, '/doc', parseQuery(queryStr)) + let queryObj = parseQuery(queryStr) + if (null === queryObj) queryObj = {} + app.render(req, res, '/doc', queryObj) } else { // Regular Next.js handler From b85eb64a0c464183c0d2c2eb1bf24f1da5a80c36 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 13:07:55 -0600 Subject: [PATCH 4/8] server: attempt to fix null query bug (2) per https://github.com/iterative/dvc.org/pull/1009#issuecomment-588748865 ref https://github.com/zeit/next.js/issues/7303#issuecomment-511053550 --- server.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server.js b/server.js index a52a96add4..e73e209571 100644 --- a/server.js +++ b/server.js @@ -25,7 +25,7 @@ const port = process.env.PORT || 3000 app.prepare().then(() => { createServer((req, res) => { const parsedUrl = parseURL(req.url) - const { pathname, queryStr } = parsedUrl + const { pathname, query: queryStr } = parsedUrl const host = req.headers.host let [redirectCode, redirectLocation] = getRedirect(host, pathname, { @@ -52,9 +52,7 @@ app.prepare().then(() => { } // Custom route for all docs - let queryObj = parseQuery(queryStr) - if (null === queryObj) queryObj = {} - app.render(req, res, '/doc', queryObj) + app.render(req, res, '/doc', parseQuery(queryStr)) } else { // Regular Next.js handler From e51bc6432e2b093e1fd46954d67a077c9657b141 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 13:20:12 -0600 Subject: [PATCH 5/8] server: attempt to fix null query bug (3) per https://github.com/iterative/dvc.org/pull/1009#issuecomment-588748865 ref https://github.com/zeit/next.js/issues/7303#issuecomment-511053550 --- server.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server.js b/server.js index e73e209571..a0856323a5 100644 --- a/server.js +++ b/server.js @@ -24,7 +24,7 @@ const port = process.env.PORT || 3000 app.prepare().then(() => { createServer((req, res) => { - const parsedUrl = parseURL(req.url) + let parsedUrl = parseURL(req.url) const { pathname, query: queryStr } = parsedUrl const host = req.headers.host @@ -56,6 +56,7 @@ app.prepare().then(() => { } else { // Regular Next.js handler + if (null === parsedUrl.query) parsedUrl.query = {} handle(req, res, parsedUrl) } }).listen(port, err => { From ed28ab48ceb95018b2340f526550ce01fdab5c7c Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 13:26:24 -0600 Subject: [PATCH 6/8] test: revert teapot eastern egg per https://github.com/iterative/dvc.org/pull/1009#discussion_r382127007 --- src/utils/redirects.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/redirects.test.js b/src/utils/redirects.test.js index 2d77809ef8..acee45db81 100644 --- a/src/utils/redirects.test.js +++ b/src/utils/redirects.test.js @@ -3,12 +3,12 @@ const { processRedirectString, getRedirect } = require('./redirects') describe('processRedirectString', () => { it('reads the regex, replacement and code', () => { - const { regex, replace, code } = processRedirectString('^/foo /bar 308') + const { regex, replace, code } = processRedirectString('^/foo /bar 418') expect(regex).toBeInstanceOf(RegExp) expect(regex.source).toEqual('^\\/foo') expect(replace).toEqual('/bar') - expect(code).toEqual(308) + expect(code).toEqual(418) }) it('defaults to 301 response code', () => { From 1e31ba876ae86495fc225f9ff8617900636247e2 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 17:16:47 -0600 Subject: [PATCH 7/8] server: revert URL query parsing optimization per https://github.com/iterative/dvc.org/pull/1009#pullrequestreview-362204807 --- server.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/server.js b/server.js index a0856323a5..84599ec34a 100644 --- a/server.js +++ b/server.js @@ -10,8 +10,7 @@ */ const { createServer } = require('http') -const { parse: parseURL } = require('url') -const { parse: parseQuery } = require('querystring') +const { parse } = require('url') const next = require('next') const { getItemByPath } = require('./src/utils/sidebar') @@ -24,8 +23,8 @@ const port = process.env.PORT || 3000 app.prepare().then(() => { createServer((req, res) => { - let parsedUrl = parseURL(req.url) - const { pathname, query: queryStr } = parsedUrl + const parsedUrl = parse(req.url, true) + const { pathname, query } = parsedUrl const host = req.headers.host let [redirectCode, redirectLocation] = getRedirect(host, pathname, { @@ -35,9 +34,13 @@ app.prepare().then(() => { if (redirectLocation) { // HTTP redirects + console.log(redirectLocation) + const { query: queryStr } = parse(req.url) + console.log(queryStr) if (queryStr) { redirectLocation += '?' + queryStr } + console.log(redirectLocation) res.writeHead(redirectCode, { 'Cache-control': 'no-cache', Location: redirectLocation @@ -52,11 +55,10 @@ app.prepare().then(() => { } // Custom route for all docs - app.render(req, res, '/doc', parseQuery(queryStr)) + app.render(req, res, '/doc', query) } else { // Regular Next.js handler - if (null === parsedUrl.query) parsedUrl.query = {} handle(req, res, parsedUrl) } }).listen(port, err => { From 08522d88d5492d7a46b80a930c74d1042cd1ac19 Mon Sep 17 00:00:00 2001 From: Jorge Orpinel Date: Thu, 20 Feb 2020 17:19:50 -0600 Subject: [PATCH 8/8] server: foud better URL query optimization (and removed accidental debug output) --- server.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server.js b/server.js index 84599ec34a..70b057758c 100644 --- a/server.js +++ b/server.js @@ -11,6 +11,7 @@ const { createServer } = require('http') const { parse } = require('url') +const { stringify } = require('querystring') const next = require('next') const { getItemByPath } = require('./src/utils/sidebar') @@ -34,13 +35,10 @@ app.prepare().then(() => { if (redirectLocation) { // HTTP redirects - console.log(redirectLocation) - const { query: queryStr } = parse(req.url) - console.log(queryStr) + const queryStr = stringify(query) if (queryStr) { redirectLocation += '?' + queryStr } - console.log(redirectLocation) res.writeHead(redirectCode, { 'Cache-control': 'no-cache', Location: redirectLocation