diff --git a/src/components/Link.js b/src/components/Link.js index 6639a65b..565117df 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, withBasePath } from 'src/utils/navigationUtils' const useStyles = makeStyles(() => ({ anchor: { @@ -15,10 +16,29 @@ const useStyles = makeStyles(() => ({ const Link = (props) => { const { children, className, to } = 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(() => { + // "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) + if (isURLForDifferentApp(url)) { + setDestInternal(false) + } + }, [to]) return ( - {children} + + {children} + ) } diff --git a/src/components/__tests__/Link.test.js b/src/components/__tests__/Link.test.js index acea53c4..5acb862a 100644 --- a/src/components/__tests__/Link.test.js +++ b/src/components/__tests__/Link.test.js @@ -1,14 +1,24 @@ import React from 'react' -import { shallow } from 'enzyme' +import { mount, shallow } from 'enzyme' import NextJsLink from 'next/link' +import { isURLForDifferentApp, withBasePath } 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/', +}) + +beforeEach(() => { + withBasePath.mockImplementation((url) => url) +}) + +afterEach(() => { + jest.clearAllMocks() }) describe('Link component', () => { @@ -54,4 +64,36 @@ 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') + }) + + 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`) + }) }) diff --git a/src/utils/__tests__/navigation.test.js b/src/utils/__tests__/navigation.test.js index 79e40b39..2c069c7e 100644 --- a/src/utils/__tests__/navigation.test.js +++ b/src/utils/__tests__/navigation.test.js @@ -1,9 +1,9 @@ /* 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') +jest.mock('src/utils/navigationUtils') // Strategy for testing against `window.location`: // https://www.benmvp.com/blog/mocking-window-location-methods-jest-jsdom/ diff --git a/src/utils/__tests__/navigationUtils.test.js b/src/utils/__tests__/navigationUtils.test.js new file mode 100644 index 00000000..b4cee120 --- /dev/null +++ b/src/utils/__tests__/navigationUtils.test.js @@ -0,0 +1,131 @@ +import setWindowLocation from 'src/utils/testHelpers/setWindowLocation' + +beforeAll(() => { + process.env.REACT_APP_WEBSITE_PROTOCOL = 'https' +}) + +afterEach(() => { + jest.clearAllMocks() + jest.resetModules() +}) + +describe('isURLForDifferentApp', () => { + 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/newtab/path/', + origin: 'https://example.com', + pathname: '/newtab/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('https://example.com/newtab/path/')).toBe(false) + }) + + 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', + hostname: 'example.com', + href: 'https://example.com/newtab/path/', + origin: 'https://example.com', + pathname: '/newtab/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('https://foo.com/newtab/path/')).toBe(true) + }) + + 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/newtab/path/', + origin: 'https://example.com', + pathname: '/newtab/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('https://example.com/something/path/')).toBe( + true + ) + }) + + 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/newtab/path/', + origin: 'https://example.com', + pathname: '/newtab/path/', + port: '', + protocol: 'https:', + search: '', + }) + expect(isURLForDifferentApp('/newtab/path/')).toBe(false) + }) + + 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', + 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) + }) +}) + +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) + }) +}) + +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/navigation.js b/src/utils/navigation.js index fda8a917..78969707 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 new file mode 100644 index 00000000..c4cc6c25 --- /dev/null +++ b/src/utils/navigationUtils.js @@ -0,0 +1,32 @@ +/* globals window */ +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 = (url) => + isAbsoluteURL(url) ? url : `${basePath}${url}` + +/** + * 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) => { + const newURLObj = new URL(newURL, window.location.origin) + + // If the URLs are on different domains, they're different apps. + if (newURLObj.hostname !== window.location.hostname) { + return true + } + + // 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] + return newURLFirstSubpath !== APP_SUBPATH +} diff --git a/src/utils/testHelpers/__tests__/setWindowLocation.test.js b/src/utils/testHelpers/__tests__/setWindowLocation.test.js new file mode 100644 index 00000000..6534e167 --- /dev/null +++ b/src/utils/testHelpers/__tests__/setWindowLocation.test.js @@ -0,0 +1,11 @@ +/* globals window */ + +test('setWindowLocation modifies the window.location object', () => { + const setWindowLocation = require('src/utils/testHelpers/setWindowLocation') + .default + 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/setWindowLocation.js b/src/utils/testHelpers/setWindowLocation.js new file mode 100644 index 00000000..167d662d --- /dev/null +++ b/src/utils/testHelpers/setWindowLocation.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} + */ +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 diff --git a/src/utils/urls.js b/src/utils/urls.js index 7edabecd..3d4c6528 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}`)