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

feat(gatsby): Add support for relative links #24054

Merged
merged 18 commits into from
Jun 2, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 79 additions & 47 deletions packages/gatsby-link/src/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import PropTypes from "prop-types"
import React from "react"
import { Link } from "@reach/router"
import { Link, Location } from "@reach/router"
import { resolve } from "@reach/router/lib/utils"

import { parsePath } from "./parse-path"

export { parsePath }

function isRelativePath(path) {
return /^\.{1,2}\//.test(path)
}

export function withPrefix(path) {
if (isRelativePath(path)) {
return normalizePath(path)
}
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
return normalizePath(
[
typeof __BASE_PATH__ !== `undefined` ? __BASE_PATH__ : __PATH_PREFIX__,
Expand All @@ -16,13 +24,23 @@ export function withPrefix(path) {
}

export function withAssetPrefix(path) {
return [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)
return isRelativePath(path)
? path
: [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)
Copy link
Contributor

@pvdz pvdz May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your code, but if you're willing to drop the regex:

There's no need for the regex here. Not sure if the absolute path can safely assumed to start with / after the isRelative check, otherwise you can simplify that logic to only .slice(1) there. (That func only tests the opposite.)

Suggested change
: [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)
: `${__PATH_PREFIX__}/${path.startsWith('/')?path.slice(1):path`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort of but there's browser support to consider for startsWith. It's a better API (that may not have existed or been implemented broadly at the time of this code change), but I'm not convinced it's worth making the change when it's functionally the same thing and more supported.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume we're polyfilling it already

Copy link
Contributor

@DSchau DSchau May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 are we comfortable going forward with an assumption? What's the benefit of changing this code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I said I assumed it would be polyfilled is because the docs say we automatically include them. The benefit is performance. Regexes are really expensive. We could in theory replace it with a substring comparison instead, but that's a lot less readable, and by the time we've used it a few times will be larger than the polyfill (and unlike the polyfill it won't be removed if people target new browsers).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My other suggestions use startsWith and endsWith as well, fwiw.

If this is really a concern then yeah, don't do it. I too thought the polyfills are in place for this.

}

function normalizePath(path) {
return path.replace(/\/+/g, `/`)
}

function absolutify(path, current) {
// If it's already absolute, return as-is
if (!isRelativePath(path)) {
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
return path
}
return resolve(path, current)
}

const NavLinkPropTypes = {
activeClassName: PropTypes.string,
activeStyle: PropTypes.object,
Expand Down Expand Up @@ -131,59 +149,73 @@ class GatsbyLink extends React.Component {
/* eslint-enable no-unused-vars */
...rest
} = this.props

const LOCAL_URL = /^\/(?!\/)/
if (process.env.NODE_ENV !== `production` && !LOCAL_URL.test(to)) {
const isRelative = isRelativePath(to)
if (
process.env.NODE_ENV !== `production` &&
!LOCAL_URL.test(to) &&
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
!isRelative
) {
console.warn(
`External link ${to} was detected in a Link component. Use the Link component only for internal links. See: https://gatsby.dev/internal-links`
)
}

const prefixedTo = withPrefix(to)

return (
<Link
to={prefixedTo}
state={state}
getProps={getProps}
innerRef={this.handleRef}
onMouseEnter={e => {
if (onMouseEnter) {
onMouseEnter(e)
}
___loader.hovering(parsePath(to).pathname)
}}
onClick={e => {
if (onClick) {
onClick(e)
}

if (
e.button === 0 && // ignore right clicks
!this.props.target && // let browser handle "target=_blank"
!e.defaultPrevented && // onClick prevented default
!e.metaKey && // ignore clicks with modifier keys...
!e.altKey &&
!e.ctrlKey &&
!e.shiftKey
) {
e.preventDefault()

let shouldReplace = replace
const isCurrent = encodeURI(to) === window.location.pathname
if (typeof replace !== `boolean` && isCurrent) {
shouldReplace = true
}

// Make sure the necessary scripts and data are
// loaded before continuing.
navigate(to, { state, replace: shouldReplace })
}

return true
<Location>
{({ location }) => {
const prefixedTo = isRelative
? absolutify(to, location.pathname)
: withPrefix(to)

return (
<Link
to={prefixedTo}
state={state}
getProps={getProps}
innerRef={this.handleRef}
onMouseEnter={e => {
if (onMouseEnter) {
onMouseEnter(e)
}
___loader.hovering(parsePath(to).pathname)
}}
onClick={e => {
if (onClick) {
onClick(e)
}

if (
e.button === 0 && // ignore right clicks
!this.props.target && // let browser handle "target=_blank"
!e.defaultPrevented && // onClick prevented default
!e.metaKey && // ignore clicks with modifier keys...
!e.altKey &&
!e.ctrlKey &&
!e.shiftKey
) {
e.preventDefault()

let shouldReplace = replace
const isCurrent = encodeURI(to) === window.location.pathname
if (typeof replace !== `boolean` && isCurrent) {
shouldReplace = true
}
// Make sure the necessary scripts and data are
// loaded before continuing.
window.___navigate(prefixedTo, {
state,
replace: shouldReplace,
})
}

return true
}}
{...rest}
/>
)
}}
{...rest}
/>
</Location>
)
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/gatsby/cache-dir/__tests__/strip-prefix.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,8 @@ describe(`strip-prefix`, () => {
it(`returns input str if no prefix is provided`, () => {
expect(stripPrefix(`/bar`)).toBe(`/bar`)
})

it(`returns "/" if str equals prefix`, () => {
expect(stripPrefix(`/bar`, `/bar`)).toBe(`/bar`)
})
})
15 changes: 12 additions & 3 deletions packages/gatsby/cache-dir/find-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ const trimPathname = rawPathname => {
return trimmedPathname
}

function absolutify(path) {
// If it's already absolute, return as-is
if (!/^\.{1,2}\/(?!\/)/.test(path)) {
return path
}
// Calculate path relative to current location, adding a trailing slash to
// match behavior of @reach/router
return new URL(path, window.location.href.replace(/([^/])$/, `$1/`)).pathname
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL constructor is not supported in IE11. https://caniuse.com/#search=url, does polyfiling take care of that for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wondered about that, but our docs says that we have babel auto-polyfills so I guessed it was ok

ascorbic marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Set list of matchPaths
*
Expand Down Expand Up @@ -55,8 +65,7 @@ export const findMatchPath = rawPathname => {
// Or if `match-paths.json` contains `{ "/foo*": "/page1", ...}`, then
// `/foo?bar=far` => `/page1`
export const findPath = rawPathname => {
const trimmedPathname = trimPathname(rawPathname)

const trimmedPathname = trimPathname(absolutify(rawPathname))
if (pathCache.has(trimmedPathname)) {
return pathCache.get(trimmedPathname)
}
Expand All @@ -80,7 +89,7 @@ export const findPath = rawPathname => {
* @return {string}
*/
export const cleanPath = rawPathname => {
const trimmedPathname = trimPathname(rawPathname)
const trimmedPathname = trimPathname(absolutify(rawPathname))

let foundPath = trimmedPathname
if (foundPath === `/index.html`) {
Expand Down
10 changes: 6 additions & 4 deletions packages/gatsby/cache-dir/strip-prefix.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@
* isn't found.
*/

export default (str, prefix = ``) => {
export default function stripPrefix(str, prefix = ``) {
if (!prefix) {
return str
}

prefix += `/`
if (str === prefix) {
return `/`
}

if (str.substr(0, prefix.length) === prefix) {
return str.slice(prefix.length - 1)
if (str.startsWith(`${prefix}/`)) {
return str.slice(prefix.length)
}

return str
Expand Down