From 1267004c9937f639941b67ff0a9867d46abc13d4 Mon Sep 17 00:00:00 2001 From: jedtan Date: Mon, 22 Mar 2021 23:58:37 -0700 Subject: [PATCH 1/9] TFAC-294 Setting target=_top for External Links --- src/components/Link.js | 18 +- src/components/__tests__/Link.test.js | 27 ++- src/utils/__tests__/navigationUtils.test.js | 179 ++++++++++++++++++ src/utils/navigationUtils.js | 43 +++++ .../testHelpers/__tests__/testUtils.test.js | 10 + src/utils/testHelpers/testUtils.js | 19 ++ 6 files changed, 292 insertions(+), 4 deletions(-) create mode 100644 src/utils/__tests__/navigationUtils.test.js create mode 100644 src/utils/navigationUtils.js create mode 100644 src/utils/testHelpers/__tests__/testUtils.test.js create mode 100644 src/utils/testHelpers/testUtils.js diff --git a/src/components/Link.js b/src/components/Link.js index 6639a65b..10e19aee 100644 --- a/src/components/Link.js +++ b/src/components/Link.js @@ -1,8 +1,9 @@ -import React from 'react' +import React, { useEffect, useState } from 'react' import PropTypes from 'prop-types' import clsx from 'clsx' import NextJsLink from 'next/link' import { makeStyles } from '@material-ui/core/styles' +import { isURLForDifferentApp } from 'src/utils/navigationUtils' const useStyles = makeStyles(() => ({ anchor: { @@ -15,10 +16,23 @@ const useStyles = makeStyles(() => ({ const Link = (props) => { const { children, className, to } = props const classes = useStyles() + const [destInternal, setDestInternal] = useState(true) + + useEffect(() => { + if (isURLForDifferentApp(to)) { + // Set that we can render the Firebase auth UI. + setDestInternal(false) + } + }, [to]) return ( - {children} + + {children} + ) } diff --git a/src/components/__tests__/Link.test.js b/src/components/__tests__/Link.test.js index acea53c4..40592229 100644 --- a/src/components/__tests__/Link.test.js +++ b/src/components/__tests__/Link.test.js @@ -1,14 +1,16 @@ import React from 'react' -import { shallow } from 'enzyme' +import { mount, shallow } from 'enzyme' import NextJsLink from 'next/link' +import { isURLForDifferentApp } from 'src/utils/navigationUtils' jest.mock('next/link') jest.mock('src/utils/urls') +jest.mock('src/utils/navigationUtils') const getMockProps = () => ({ children: 'hi', className: 'some-class', - to: '/some/url', + to: 'https://tab.gladly.io/newtab/blah/', }) describe('Link component', () => { @@ -54,4 +56,25 @@ describe('Link component', () => { const anchor = wrapper.childAt(0) expect(anchor.prop('className')).toContain('some-class-here') }) + + it('has no target prop on anchor tag by default', () => { + const Link = require('src/components/Link').default + + const wrapper = shallow() + const anchor = wrapper.find('a').first() + expect(anchor.prop('target')).toBeUndefined() + }) + + it('sets target to _top for external app', () => { + isURLForDifferentApp.mockReturnValue(true) + const Link = require('src/components/Link').default + + const mockProps = { + ...getMockProps(), + url: 'https://blahblah.com', + } + const wrapper = mount() + const anchor = wrapper.find('a').first() + expect(anchor.prop('target')).toEqual('_top') + }) }) diff --git a/src/utils/__tests__/navigationUtils.test.js b/src/utils/__tests__/navigationUtils.test.js new file mode 100644 index 00000000..a71cc186 --- /dev/null +++ b/src/utils/__tests__/navigationUtils.test.js @@ -0,0 +1,179 @@ +import { setWindowLocation } from 'src/utils/testHelpers/testUtils' + +beforeAll(() => { + process.env.REACT_APP_WEBSITE_PROTOCOL = 'https' +}) + +afterEach(() => { + jest.clearAllMocks() +}) + +describe('isURLForDifferentApp', () => { + it('[absolute URL] considers the apps different if the URL is absolute, even if the URL is exactly the same', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/blah/path/', + origin: 'https://example.com', + pathname: '/blah/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('https://example.com/blah/path/')).toBe(true) + }) + + it('[absolute URL] considers the apps different if the URL is absolute and the URL is on a different app subpath', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/newtab/path/', + origin: 'https://example.com', + pathname: '/newtab/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('https://example.com/search/path/')).toBe(true) + }) + + it('[relative URL] considers the apps the same if the URL is exactly the same', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/blah/path/', + origin: 'https://example.com', + pathname: '/blah/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/blah/path/')).toBe(false) + }) + + it('[relative URL] considers the apps the same if the URLs are pages on the same "homepage" app (not part of the "newtab" or "search" subpaths)', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/blah/path/', + origin: 'https://example.com', + pathname: '/blah/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/some/thing/')).toBe(false) + }) + + it('[relative URL] considers the apps the same if the URLs are both on the "newtab" subpath', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/newtab/thing/', + origin: 'https://example.com', + pathname: '/newtab/thing/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/newtab/foobar/')).toBe(false) + }) + + it('[relative URL] considers the apps the same if the URLs are both on the "search" subpath', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/search/thing/', + origin: 'https://example.com', + pathname: '/search/thing/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/search/foobar/')).toBe(false) + }) + + it('[relative URL] considers the apps different if the current location is on the "search" subpath and the URL is for the "homepage" app', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/search/thing/', + origin: 'https://example.com', + pathname: '/search/thing/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/my-homepage/')).toBe(true) + }) + + it('[relative URL] considers the apps different if the current location is on the "search" subpath and the URL is for the "newtab" app', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/search/thing/', + origin: 'https://example.com', + pathname: '/search/thing/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/newtab/foobar/')).toBe(true) + }) + + it('[relative URL] considers the apps different if the current location is on the "newtab" subpath and the URL is for the "homepage" app', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/newtab/thing/', + origin: 'https://example.com', + pathname: '/newtab/thing/', + port: '', + protocol: 'https:', + newtab: '', + }) + expect(isURLForDifferentApp('/my-homepage/')).toBe(true) + }) + + it('[relative URL] considers the apps different if the current location is on the "newtab" subpath and the URL is for the "search" app', () => { + const { isURLForDifferentApp } = require('src/utils/navigationUtils') + setWindowLocation({ + host: 'example.com', + hostname: 'example.com', + href: 'https://example.com/newtab/thing/', + origin: 'https://example.com', + pathname: '/newtab/thing/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/search/foobar/')).toBe(true) + }) +}) + +describe('isAbsoluteURL', () => { + it('works as expected', () => { + const { isAbsoluteURL } = require('src/utils/navigationUtils') + + // Relative URLs + expect(isAbsoluteURL('')).toBe(false) + expect(isAbsoluteURL('/')).toBe(false) + expect(isAbsoluteURL('/some/path')).toBe(false) + expect(isAbsoluteURL('localhost:3000')).toBe(false) + + // Absolute URLs + expect(isAbsoluteURL('https://example.com/blah/path/')).toBe(true) + expect(isAbsoluteURL('http://example.com/blah/path/')).toBe(true) + expect(isAbsoluteURL('https://example.com')).toBe(true) + expect(isAbsoluteURL('http://localhost:3000')).toBe(true) + }) +}) diff --git a/src/utils/navigationUtils.js b/src/utils/navigationUtils.js new file mode 100644 index 00000000..e85182e6 --- /dev/null +++ b/src/utils/navigationUtils.js @@ -0,0 +1,43 @@ +/* globals window */ +export const isAbsoluteURL = (url) => + !!(url.startsWith('http://') || url.startsWith('https://')) + +/** + * Determine whether a URL is for a different app from the current + * URL. In other words, navigating to the new URL cannot simply + * navigate within a single-page app. + * @param {String} newURL - The URL to test + * @return {Boolean} Whether the new URL is for another app + */ +export const isURLForDifferentApp = (newURL) => { + // If the URL is absolute, treat it as external. + if (isAbsoluteURL(newURL)) { + return true + } + + const newURLObj = new URL(newURL, window.location.origin) + + // If the URLs are on different domains, they're different apps. + const isSameDomain = newURLObj.hostname === window.location.hostname + if (!isSameDomain) { + return true + } + + // The first-level paths at which we host separate apps. Any other + // paths are part of the "homepage" app. + const differentAppSubpaths = ['newtab', 'search'] + + // If the new URL or current URL are on one of our app subpaths, and + // they aren't on the same subpath, they're on different apps. + const newURLFirstSubpath = newURLObj.pathname.split('/')[1] + const currentURLFirstSubpath = window.location.pathname.split('/')[1] + if ( + differentAppSubpaths.indexOf(newURLFirstSubpath) > -1 || + differentAppSubpaths.indexOf(currentURLFirstSubpath) > -1 + ) { + return newURLFirstSubpath !== currentURLFirstSubpath + } + + // Otherwise, assume we're on the same app. + return false +} diff --git a/src/utils/testHelpers/__tests__/testUtils.test.js b/src/utils/testHelpers/__tests__/testUtils.test.js new file mode 100644 index 00000000..8bdfb1eb --- /dev/null +++ b/src/utils/testHelpers/__tests__/testUtils.test.js @@ -0,0 +1,10 @@ +/* globals window */ + +test('setWindowLocation modifies the window.location object', () => { + const { setWindowLocation } = require('src/utils/testHelpers/testUtils') + expect(window.location.hostname).not.toEqual('foo.com') + setWindowLocation({ + hostname: 'foo.com', + }) + expect(window.location.hostname).toEqual('foo.com') +}) diff --git a/src/utils/testHelpers/testUtils.js b/src/utils/testHelpers/testUtils.js new file mode 100644 index 00000000..d650d513 --- /dev/null +++ b/src/utils/testHelpers/testUtils.js @@ -0,0 +1,19 @@ +/* globals window */ + +/** + * Change the window.location object, merging with existing values. + * @param {Object} modifiedLocation - An object with any values + * to modify on the window.location object. + * @return {undefined} + */ +export const setWindowLocation = (modifiedLocation) => { + const windowLocation = JSON.stringify(window.location) + delete window.location + Object.defineProperty(window, 'location', { + value: { ...JSON.parse(windowLocation), ...modifiedLocation }, + configurable: true, + writable: true, + }) +} + +export default setWindowLocation From 94b05b490d5c01001f2a3614cf0577a7e18e332f Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 11:32:21 -0700 Subject: [PATCH 2/9] Simplify "different app" logic, rename file, add comment --- src/components/Link.js | 4 +- src/utils/__tests__/navigationUtils.test.js | 109 +++--------------- src/utils/navigationUtils.js | 28 +---- ...tils.test.js => setWindowLocation.test.js} | 3 +- .../{testUtils.js => setWindowLocation.js} | 2 +- 5 files changed, 29 insertions(+), 117 deletions(-) rename src/utils/testHelpers/__tests__/{testUtils.test.js => setWindowLocation.test.js} (73%) rename src/utils/testHelpers/{testUtils.js => setWindowLocation.js} (89%) diff --git a/src/components/Link.js b/src/components/Link.js index 10e19aee..c1f584d5 100644 --- a/src/components/Link.js +++ b/src/components/Link.js @@ -18,9 +18,11 @@ const Link = (props) => { const classes = useStyles() const [destInternal, setDestInternal] = useState(true) + // We won't always server-render the "target" prop correctly. + // If that causes problems, use the URL from the request when + // server-side rendering. useEffect(() => { if (isURLForDifferentApp(to)) { - // Set that we can render the Firebase auth UI. setDestInternal(false) } }, [to]) diff --git a/src/utils/__tests__/navigationUtils.test.js b/src/utils/__tests__/navigationUtils.test.js index a71cc186..fb6f5caa 100644 --- a/src/utils/__tests__/navigationUtils.test.js +++ b/src/utils/__tests__/navigationUtils.test.js @@ -1,4 +1,4 @@ -import { setWindowLocation } from 'src/utils/testHelpers/testUtils' +import setWindowLocation from 'src/utils/testHelpers/setWindowLocation' beforeAll(() => { process.env.REACT_APP_WEBSITE_PROTOCOL = 'https' @@ -9,22 +9,22 @@ afterEach(() => { }) describe('isURLForDifferentApp', () => { - it('[absolute URL] considers the apps different if the URL is absolute, even if the URL is exactly the same', () => { + it('[absolute URL] is not a different app when the URL is absolute with the same domain and a "newtab" path', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', hostname: 'example.com', - href: 'https://example.com/blah/path/', + href: 'https://example.com/newtab/path/', origin: 'https://example.com', - pathname: '/blah/path/', + pathname: '/newtab/path/', port: '', protocol: 'https:', search: '', }) - expect(isURLForDifferentApp('https://example.com/blah/path/')).toBe(true) + expect(isURLForDifferentApp('https://example.com/newtab/path/')).toBe(false) }) - it('[absolute URL] considers the apps different if the URL is absolute and the URL is on a different app subpath', () => { + it('[absolute URL] is a different app when the URL is absolute with a different domain', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', @@ -36,40 +36,42 @@ describe('isURLForDifferentApp', () => { protocol: 'https:', search: '', }) - expect(isURLForDifferentApp('https://example.com/search/path/')).toBe(true) + expect(isURLForDifferentApp('https://foo.com/newtab/path/')).toBe(true) }) - it('[relative URL] considers the apps the same if the URL is exactly the same', () => { + it('[absolute URL] is a different app when the URL is absolute and on the same domain but with a non-"newtab" app subpath', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', hostname: 'example.com', - href: 'https://example.com/blah/path/', + href: 'https://example.com/newtab/path/', origin: 'https://example.com', - pathname: '/blah/path/', + pathname: '/newtab/path/', port: '', protocol: 'https:', search: '', }) - expect(isURLForDifferentApp('/blah/path/')).toBe(false) + expect(isURLForDifferentApp('https://example.com/something/path/')).toBe( + true + ) }) - it('[relative URL] considers the apps the same if the URLs are pages on the same "homepage" app (not part of the "newtab" or "search" subpaths)', () => { + it('[relative URL] is not a different app if the URL is exactly the same with a newtab subpath', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', hostname: 'example.com', - href: 'https://example.com/blah/path/', + href: 'https://example.com/newtab/path/', origin: 'https://example.com', - pathname: '/blah/path/', + pathname: '/newtab/path/', port: '', protocol: 'https:', search: '', }) - expect(isURLForDifferentApp('/some/thing/')).toBe(false) + expect(isURLForDifferentApp('/newtab/path/')).toBe(false) }) - it('[relative URL] considers the apps the same if the URLs are both on the "newtab" subpath', () => { + it('[relative URL] is not a differenta app if the URLs are both on the "newtab" subpath', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', @@ -83,81 +85,6 @@ describe('isURLForDifferentApp', () => { }) expect(isURLForDifferentApp('/newtab/foobar/')).toBe(false) }) - - it('[relative URL] considers the apps the same if the URLs are both on the "search" subpath', () => { - const { isURLForDifferentApp } = require('src/utils/navigationUtils') - setWindowLocation({ - host: 'example.com', - hostname: 'example.com', - href: 'https://example.com/search/thing/', - origin: 'https://example.com', - pathname: '/search/thing/', - port: '', - protocol: 'https:', - search: '', - }) - expect(isURLForDifferentApp('/search/foobar/')).toBe(false) - }) - - it('[relative URL] considers the apps different if the current location is on the "search" subpath and the URL is for the "homepage" app', () => { - const { isURLForDifferentApp } = require('src/utils/navigationUtils') - setWindowLocation({ - host: 'example.com', - hostname: 'example.com', - href: 'https://example.com/search/thing/', - origin: 'https://example.com', - pathname: '/search/thing/', - port: '', - protocol: 'https:', - search: '', - }) - expect(isURLForDifferentApp('/my-homepage/')).toBe(true) - }) - - it('[relative URL] considers the apps different if the current location is on the "search" subpath and the URL is for the "newtab" app', () => { - const { isURLForDifferentApp } = require('src/utils/navigationUtils') - setWindowLocation({ - host: 'example.com', - hostname: 'example.com', - href: 'https://example.com/search/thing/', - origin: 'https://example.com', - pathname: '/search/thing/', - port: '', - protocol: 'https:', - search: '', - }) - expect(isURLForDifferentApp('/newtab/foobar/')).toBe(true) - }) - - it('[relative URL] considers the apps different if the current location is on the "newtab" subpath and the URL is for the "homepage" app', () => { - const { isURLForDifferentApp } = require('src/utils/navigationUtils') - setWindowLocation({ - host: 'example.com', - hostname: 'example.com', - href: 'https://example.com/newtab/thing/', - origin: 'https://example.com', - pathname: '/newtab/thing/', - port: '', - protocol: 'https:', - newtab: '', - }) - expect(isURLForDifferentApp('/my-homepage/')).toBe(true) - }) - - it('[relative URL] considers the apps different if the current location is on the "newtab" subpath and the URL is for the "search" app', () => { - const { isURLForDifferentApp } = require('src/utils/navigationUtils') - setWindowLocation({ - host: 'example.com', - hostname: 'example.com', - href: 'https://example.com/newtab/thing/', - origin: 'https://example.com', - pathname: '/newtab/thing/', - port: '', - protocol: 'https:', - search: '', - }) - expect(isURLForDifferentApp('/search/foobar/')).toBe(true) - }) }) describe('isAbsoluteURL', () => { diff --git a/src/utils/navigationUtils.js b/src/utils/navigationUtils.js index e85182e6..8e75b84a 100644 --- a/src/utils/navigationUtils.js +++ b/src/utils/navigationUtils.js @@ -10,34 +10,16 @@ export const isAbsoluteURL = (url) => * @return {Boolean} Whether the new URL is for another app */ export const isURLForDifferentApp = (newURL) => { - // If the URL is absolute, treat it as external. - if (isAbsoluteURL(newURL)) { - return true - } - const newURLObj = new URL(newURL, window.location.origin) // If the URLs are on different domains, they're different apps. - const isSameDomain = newURLObj.hostname === window.location.hostname - if (!isSameDomain) { + if (newURLObj.hostname !== window.location.hostname) { return true } - // The first-level paths at which we host separate apps. Any other - // paths are part of the "homepage" app. - const differentAppSubpaths = ['newtab', 'search'] - - // If the new URL or current URL are on one of our app subpaths, and - // they aren't on the same subpath, they're on different apps. + // If the new URL is not on the "newtab" subpath, it is a + // different app. + const APP_SUBPATH = 'newtab' const newURLFirstSubpath = newURLObj.pathname.split('/')[1] - const currentURLFirstSubpath = window.location.pathname.split('/')[1] - if ( - differentAppSubpaths.indexOf(newURLFirstSubpath) > -1 || - differentAppSubpaths.indexOf(currentURLFirstSubpath) > -1 - ) { - return newURLFirstSubpath !== currentURLFirstSubpath - } - - // Otherwise, assume we're on the same app. - return false + return newURLFirstSubpath !== APP_SUBPATH } diff --git a/src/utils/testHelpers/__tests__/testUtils.test.js b/src/utils/testHelpers/__tests__/setWindowLocation.test.js similarity index 73% rename from src/utils/testHelpers/__tests__/testUtils.test.js rename to src/utils/testHelpers/__tests__/setWindowLocation.test.js index 8bdfb1eb..6534e167 100644 --- a/src/utils/testHelpers/__tests__/testUtils.test.js +++ b/src/utils/testHelpers/__tests__/setWindowLocation.test.js @@ -1,7 +1,8 @@ /* globals window */ test('setWindowLocation modifies the window.location object', () => { - const { setWindowLocation } = require('src/utils/testHelpers/testUtils') + const setWindowLocation = require('src/utils/testHelpers/setWindowLocation') + .default expect(window.location.hostname).not.toEqual('foo.com') setWindowLocation({ hostname: 'foo.com', diff --git a/src/utils/testHelpers/testUtils.js b/src/utils/testHelpers/setWindowLocation.js similarity index 89% rename from src/utils/testHelpers/testUtils.js rename to src/utils/testHelpers/setWindowLocation.js index d650d513..167d662d 100644 --- a/src/utils/testHelpers/testUtils.js +++ b/src/utils/testHelpers/setWindowLocation.js @@ -6,7 +6,7 @@ * to modify on the window.location object. * @return {undefined} */ -export const setWindowLocation = (modifiedLocation) => { +const setWindowLocation = (modifiedLocation) => { const windowLocation = JSON.stringify(window.location) delete window.location Object.defineProperty(window, 'location', { From ddfab489b3f552cf758f070b99b18205d60a2159 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 12:25:16 -0700 Subject: [PATCH 3/9] Add FIXME --- src/components/Link.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/components/Link.js b/src/components/Link.js index c1f584d5..b9cc759c 100644 --- a/src/components/Link.js +++ b/src/components/Link.js @@ -5,6 +5,8 @@ import NextJsLink from 'next/link' import { makeStyles } from '@material-ui/core/styles' import { isURLForDifferentApp } from 'src/utils/navigationUtils' +// import { withBasePath } from 'src/utils/urls' + const useStyles = makeStyles(() => ({ anchor: { display: 'inline-block', @@ -22,7 +24,13 @@ const Link = (props) => { // If that causes problems, use the URL from the request when // server-side rendering. useEffect(() => { - if (isURLForDifferentApp(to)) { + // FIXME + // "When linking to other pages using next/link and next/router + // the basePath will be automatically applied." + // https://nextjs.org/docs/api-reference/next.config.js/basepath#links + // const url = withBasePath(to) + const url = to + if (isURLForDifferentApp(url)) { setDestInternal(false) } }, [to]) From c7840d8ab3264165e0a06d4828c99a67d1030050 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 12:31:55 -0700 Subject: [PATCH 4/9] Move withBasePath --- src/components/Link.js | 2 +- src/utils/navigation.js | 2 +- src/utils/navigationUtils.js | 6 ++++++ src/utils/urls.js | 7 +------ 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/components/Link.js b/src/components/Link.js index b9cc759c..3067a07c 100644 --- a/src/components/Link.js +++ b/src/components/Link.js @@ -5,7 +5,7 @@ import NextJsLink from 'next/link' import { makeStyles } from '@material-ui/core/styles' import { isURLForDifferentApp } from 'src/utils/navigationUtils' -// import { withBasePath } from 'src/utils/urls' +// import { withBasePath } from 'src/utils/navigationUtils' const useStyles = makeStyles(() => ({ anchor: { diff --git a/src/utils/navigation.js b/src/utils/navigation.js index 3d562d10..9bd5d7c5 100644 --- a/src/utils/navigation.js +++ b/src/utils/navigation.js @@ -1,7 +1,7 @@ /* globals window */ import Router from 'next/router' import { isServerSide } from 'src/utils/ssr' -import { withBasePath } from 'src/utils/urls' +import { withBasePath } from 'src/utils/navigationUtils' // Handle redirects on both the client side and server side. // Adapted from: diff --git a/src/utils/navigationUtils.js b/src/utils/navigationUtils.js index 8e75b84a..d3afe505 100644 --- a/src/utils/navigationUtils.js +++ b/src/utils/navigationUtils.js @@ -2,6 +2,12 @@ export const isAbsoluteURL = (url) => !!(url.startsWith('http://') || url.startsWith('https://')) +// Base path set in Next config. This must match our app's +// CloudFront routing. +const basePath = process.env.NEXT_PUBLIC_URLS_BASE_PATH || '' + +export const withBasePath = (path) => `${basePath}${path}` + /** * Determine whether a URL is for a different app from the current * URL. In other words, navigating to the new URL cannot simply diff --git a/src/utils/urls.js b/src/utils/urls.js index 0f92f092..5097d0ad 100644 --- a/src/utils/urls.js +++ b/src/utils/urls.js @@ -1,4 +1,5 @@ import ensureValuesAreDefined from 'src/utils/ensureValuesAreDefined' +import { withBasePath } from 'src/utils/navigationUtils' try { ensureValuesAreDefined([ @@ -12,10 +13,6 @@ try { ) } -// Base path set in Next config. This must match our app's -// CloudFront routing. -const basePath = process.env.NEXT_PUBLIC_URLS_BASE_PATH || '' - // In CloudFront, the /v4 base path routes to this Next.js // app. The /newtab base paths routes to the this app -OR- // the legacy static app, depending on cookie settings. Thus, @@ -32,8 +29,6 @@ const addTrailingSlashIfNeeded = (path) => { return `${path}${!hasTrailingSlash && useTrailingSlash ? '/' : ''}` } -export const withBasePath = (path) => `${basePath}${path}` - // For /api/* paths. const createAPIURL = (url) => addTrailingSlashIfNeeded(`${NEXT_PUBLIC_URLS_API_BASE_PATH}${url}`) From aeb353029fe8ff6d14532d07d3fcd98a661dbe16 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 12:36:34 -0700 Subject: [PATCH 5/9] Don't add the base path to absolute URLs --- src/utils/__tests__/navigationUtils.test.js | 25 +++++++++++++++++++++ src/utils/navigationUtils.js | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/utils/__tests__/navigationUtils.test.js b/src/utils/__tests__/navigationUtils.test.js index fb6f5caa..52787e23 100644 --- a/src/utils/__tests__/navigationUtils.test.js +++ b/src/utils/__tests__/navigationUtils.test.js @@ -6,6 +6,7 @@ beforeAll(() => { afterEach(() => { jest.clearAllMocks() + jest.resetModules() }) describe('isURLForDifferentApp', () => { @@ -104,3 +105,27 @@ describe('isAbsoluteURL', () => { expect(isAbsoluteURL('http://localhost:3000')).toBe(true) }) }) + +describe('withBasePath', () => { + beforeEach(() => { + process.env.NEXT_PUBLIC_URLS_BASE_PATH = '/some-path' + }) + + it('adds the basePath, when it is defined, to a relative URL', () => { + const { withBasePath } = require('src/utils/navigationUtils') + expect(withBasePath('/my-url')).toEqual('/some-path/my-url') + }) + + it('does not add the basePath, when it is not defined, to a relative URL', () => { + delete process.env.NEXT_PUBLIC_URLS_BASE_PATH + const { withBasePath } = require('src/utils/navigationUtils') + expect(withBasePath('/my-url')).toEqual('/my-url') + }) + + it('does not add the basePath to an absolute URL', () => { + const { withBasePath } = require('src/utils/navigationUtils') + expect(withBasePath('https://example.com/my-url')).toEqual( + 'https://example.com/my-url' + ) + }) +}) diff --git a/src/utils/navigationUtils.js b/src/utils/navigationUtils.js index d3afe505..c4cc6c25 100644 --- a/src/utils/navigationUtils.js +++ b/src/utils/navigationUtils.js @@ -6,7 +6,8 @@ export const isAbsoluteURL = (url) => // CloudFront routing. const basePath = process.env.NEXT_PUBLIC_URLS_BASE_PATH || '' -export const withBasePath = (path) => `${basePath}${path}` +export const withBasePath = (url) => + isAbsoluteURL(url) ? url : `${basePath}${url}` /** * Determine whether a URL is for a different app from the current From d7c6959ed81c9ff83636ef530e181659eff0b958 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 12:43:01 -0700 Subject: [PATCH 6/9] Include the base path when determining Link render state --- src/components/Link.js | 8 ++------ src/components/__tests__/Link.test.js | 21 ++++++++++++++++++++- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/components/Link.js b/src/components/Link.js index 3067a07c..565117df 100644 --- a/src/components/Link.js +++ b/src/components/Link.js @@ -3,9 +3,7 @@ import PropTypes from 'prop-types' import clsx from 'clsx' import NextJsLink from 'next/link' import { makeStyles } from '@material-ui/core/styles' -import { isURLForDifferentApp } from 'src/utils/navigationUtils' - -// import { withBasePath } from 'src/utils/navigationUtils' +import { isURLForDifferentApp, withBasePath } from 'src/utils/navigationUtils' const useStyles = makeStyles(() => ({ anchor: { @@ -24,12 +22,10 @@ const Link = (props) => { // If that causes problems, use the URL from the request when // server-side rendering. useEffect(() => { - // FIXME // "When linking to other pages using next/link and next/router // the basePath will be automatically applied." // https://nextjs.org/docs/api-reference/next.config.js/basepath#links - // const url = withBasePath(to) - const url = to + const url = withBasePath(to) if (isURLForDifferentApp(url)) { setDestInternal(false) } diff --git a/src/components/__tests__/Link.test.js b/src/components/__tests__/Link.test.js index 40592229..5acb862a 100644 --- a/src/components/__tests__/Link.test.js +++ b/src/components/__tests__/Link.test.js @@ -1,7 +1,7 @@ import React from 'react' import { mount, shallow } from 'enzyme' import NextJsLink from 'next/link' -import { isURLForDifferentApp } from 'src/utils/navigationUtils' +import { isURLForDifferentApp, withBasePath } from 'src/utils/navigationUtils' jest.mock('next/link') jest.mock('src/utils/urls') @@ -13,6 +13,14 @@ const getMockProps = () => ({ to: 'https://tab.gladly.io/newtab/blah/', }) +beforeEach(() => { + withBasePath.mockImplementation((url) => url) +}) + +afterEach(() => { + jest.clearAllMocks() +}) + describe('Link component', () => { it('renders without error', () => { const Link = require('src/components/Link').default @@ -77,4 +85,15 @@ describe('Link component', () => { const anchor = wrapper.find('a').first() expect(anchor.prop('target')).toEqual('_top') }) + + it('includes the base path when calling isURLForDifferentApp', () => { + const Link = require('src/components/Link').default + withBasePath.mockImplementation((url) => `/my-base-path${url}`) + const mockProps = { + ...getMockProps(), + to: '/my/thing', + } + mount() + expect(isURLForDifferentApp).toHaveBeenCalledWith(`/my-base-path/my/thing`) + }) }) From 2a6d510657fabcda84b32c604049b1bee385387b Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 13:35:03 -0700 Subject: [PATCH 7/9] Fix import --- src/utils/__tests__/navigation.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/__tests__/navigation.test.js b/src/utils/__tests__/navigation.test.js index 79e40b39..2bc4520f 100644 --- a/src/utils/__tests__/navigation.test.js +++ b/src/utils/__tests__/navigation.test.js @@ -1,6 +1,6 @@ /* globals window */ import { isServerSide } from 'src/utils/ssr' -import { withBasePath } from 'src/utils/urls' +import { withBasePath } from 'src/utils/navigationUtils' jest.mock('src/utils/ssr') jest.mock('src/utils/urls') From b97c5df6615fa1f8b285aae3d2a8430b4d31fc17 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 13:47:44 -0700 Subject: [PATCH 8/9] Fix mock --- src/utils/__tests__/navigation.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/__tests__/navigation.test.js b/src/utils/__tests__/navigation.test.js index 2bc4520f..2c069c7e 100644 --- a/src/utils/__tests__/navigation.test.js +++ b/src/utils/__tests__/navigation.test.js @@ -3,7 +3,7 @@ import { isServerSide } from 'src/utils/ssr' import { withBasePath } from 'src/utils/navigationUtils' jest.mock('src/utils/ssr') -jest.mock('src/utils/urls') +jest.mock('src/utils/navigationUtils') // Strategy for testing against `window.location`: // https://www.benmvp.com/blog/mocking-window-location-methods-jest-jsdom/ From 6c948437bd3e18af951b42c0680a182e222e9185 Mon Sep 17 00:00:00 2001 From: Kevin Jennison Date: Mon, 29 Mar 2021 13:58:17 -0700 Subject: [PATCH 9/9] Test name tweaks --- src/utils/__tests__/navigationUtils.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils/__tests__/navigationUtils.test.js b/src/utils/__tests__/navigationUtils.test.js index 52787e23..b4cee120 100644 --- a/src/utils/__tests__/navigationUtils.test.js +++ b/src/utils/__tests__/navigationUtils.test.js @@ -10,7 +10,7 @@ afterEach(() => { }) describe('isURLForDifferentApp', () => { - it('[absolute URL] is not a different app when the URL is absolute with the same domain and a "newtab" path', () => { + it('[absolute URL] is *not* a different app when the URL is absolute with the same domain and a "newtab" path', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', @@ -57,7 +57,7 @@ describe('isURLForDifferentApp', () => { ) }) - it('[relative URL] is not a different app if the URL is exactly the same with a newtab subpath', () => { + it('[relative URL] is *not* a different app if the URL is exactly the same with a "newtab" subpath', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com', @@ -72,7 +72,7 @@ describe('isURLForDifferentApp', () => { expect(isURLForDifferentApp('/newtab/path/')).toBe(false) }) - it('[relative URL] is not a differenta app if the URLs are both on the "newtab" subpath', () => { + it('[relative URL] is *not* a different app if the URL has a "newtab" subpath', () => { const { isURLForDifferentApp } = require('src/utils/navigationUtils') setWindowLocation({ host: 'example.com',