feat(gatsby): Add support for relative links

[ascorbic]

Currently gatsby-link doesn't support relative paths, unlike the underlying @reach/router. This is because we need to do a lot more than navigation, particularly loading of resources. This is confusing to users, and people have long requested that we add support (e.g. #6945).

This PR adds support for relative links. It does this by resolving the links, both at runtime and build time, relative to the current route. It handles pathPrefix, but I've not yet tested it with client-only routes, which is why it's a draft. It follows the @reach/router behaviour of treating relative links as if the current page has a trailing slash. i.e linking from /foo/bar to .. resolves to /foo not / as it would with a POSIX path.

[blainekasten]

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

[ascorbic]

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

[blainekasten]

This is a really awesome start! I think the direction is looking great!

[pvdz]

The hunt for regexes is on.

[pvdz]

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`

[DSchau]

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

[ascorbic]

I'd assume we're polyfilling it already

[DSchau]

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

[ascorbic]

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).

[pvdz]

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.

[pvdz]

💜 the changes, thank you :)

[pieh]

Let's make sure to add some test cases for relative links in
https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/development-runtime/cypress/integration/navigation/linking.js and https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/production-runtime/cypress/integration/1-production.js as this PR will get closer to "done"

Also please make sure that prefetching (___loader.enqueue calls) also have adjusted args - it might be a bit messy with those right now as those are not in render function so there might be need to move things around - maybe we could encapsulate relative path handling above GatsbyLink itself?

so current

export default React.forwardRef((props, ref) => (
  <GatsbyLink innerRef={ref} {...props} />
))

would be something like

export default React.forwardRef(({to, ...restOfProps}, ref) => (
<Location>
  {({ location }) => {
    const adjustedTo = magic(to, location)
    return <GatsbyLink innerRef={ref} to={adjustedTo} {...restOfProps} />
  })
</Location>
))

so GatsbyLink itself already get adjusted to prop and we don't have to make any changes there?

[ascorbic]

I have some questions about link behaviour. This table shows how it behaves right now:

From page	Link	Current	Proposed
/	/foo	/foo	/foo
/	foo	/foo*	/foo
/	./	/foo*	/foo
/foo/	./bar	/bar/*	/foo/bar/
/foo	./bar	/bar*	/foo/bar
/foo/	bar	/bar/*	/foo/bar/ ?
/foo	bar	/bar*	/foo/bar ?
/foo/bar/	baz	/baz/*	/foo/bar/baz/ ?
/foo/bar	baz	/baz*	/foo/bar/baz ?
/foo/bar/	../baz	/baz/*	/foo/baz/
/foo/bar	../baz	/baz*	/foo/baz

* Gives warning about external links
? Not sure whether it should warn

All changed behaviour was already giving a warning. There are a few slightly confusing cases, which are due to how @reach/router handles trailing slashes. Because of how ambigious they can be, Reach treats them as if they all have a trailing slash, so linking from /foo/bar to ./baz will take you to foo/bar/baz, not /foo/baz as it would in POSIX. They give their rationale here, which I find persuasive but does potentially cause confusion so would need documentation.

My main question is whether we should warn for any of these now, particularly ones that are potentially ambiguous like a bare link with no inital slash or dot like foo

[sidharthachatterjee]

@ascorbic Tests are great! Thank you for those. Do we need to test this with gatsby-plugin-offline?

[sidharthachatterjee]

Canary is out at

Successfully published:
 - [email protected]+545ff9d68
 - [email protected]+545ff9d68
lerna success published 6 packages

[sidharthachatterjee]

Looks great!

[wardpeet]

We do not have a sizebot yet 😛
Just saying, this PR adds 1.2kb do the bundle. Mostly startWith polyfill. No big deal

[sidharthachatterjee]

@wardpeet You are our sizebot 😛

[AishaBlake]

@ascorbic Pulling my Slack comment out into the open! Would you mind updating the linking between pages doc as part of this PR? Let me know if you'd like a little direction there! I just want to make sure we're correctly describing the component's behavior wherever it's mentioned.

[Ekwuno]

This looks good to me. Thanks @ascorbic for documenting.

[sidharthachatterjee]

💥

[robinmetral]

@ascorbic did this also add support for external links?

I'm asking because the docs have long warned that should only be used for local links.

I think it's a nice feature, until now I've had to implement a similar workaround in a wrapper around to render either a Gatsby Link for local links or an anchor for others.

However this PR misses two things if this really is adding support for external links:

• we need to update the docs for it
• we need to remove this warning:

gatsby/packages/gatsby-link/src/index.js

Lines 169 to 173 in be86318

	if (process.env.NODE_ENV !== `production` && !isLocalLink(to)) {
	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`
	)
	}

[ascorbic]

@robinmetral This change handles them more gracefully, but we still discourage their use, which is why we still have the warning. It means we can change the behaviour in future without making it a breaking change.

[robinmetral]

Thanks @ascorbic, got it. So as far as I understood, I can basically use Link for external links now, but I'll continue getting warnings.

What would be missing to make this a more officially supported feature? I can help if necessary, I'd love to get rid of my custom wrapper 😛

[ascorbic]

You can continue to use them, but risk having us change behaviour in the future. For example, adding relative link support broke some existing links without leading slashes, which had worked but with warnings.

[fabianhaglund]

Thank you heaps for this!