-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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(remix-react): allow <Link>
's "to" prop to accept absolute urls
#5092
Conversation
🦋 Changeset detectedLatest commit: e69f19d The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/remix-react/components.tsx
Outdated
{children} | ||
</a> | ||
{shouldPrefetch ? ( | ||
<link key={href} rel="prefetch" as="document" href={to} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need the as="document"
here, I'm using it on my blog without that and it works https://github.com/sergiodxa/sergiodxa.com/blob/ca1fa0d9c7f8d92322b01f009951bbfdd54c6214/app/root.tsx#L213-L215
Also when I tried to prefetch links to external websites the browser didn't re-used the prefetched response and did a new request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like rel=prerender
might be what we're actually after https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/prerender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any browser supporting prerender? I tried in Safari and Chromium (Edge) and it did nothing 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back working on this, looks like pre-render was removed 🫠 https://developer.chrome.com/blog/prerender-pages/#a-brief-history-of-prerender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it's a script tag apparently and idk how to feel lol
<script type="speculationrules">
{
"prerender": [
{"source": "list",
"urls": ["/home", "/about"]}
],
"prefetch": [
{"source": "list",
"urls": ["https://en.wikipedia.org/wiki/Hamster_racing"],
"requires": ["anonymous-client-ip-when-cross-origin"]}
]
}
</script>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional update: pre-render does work still in chrome, confirmed using https://stackoverflow.com/a/64330790
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to try to optimize external links to apps we don't control, especially if it's potentially not straightforward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not, but would've been nice if it was a straightforward api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah definitely
c5bf19d
to
c9cb93f
Compare
packages/remix-react/components.tsx
Outdated
let location = typeof to === "string" ? to : createPath(to); | ||
|
||
let isAbsolute = | ||
/^[a-z+]+:\/\//i.test(location) || location.startsWith("//"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same detection we do over in router land https://github.com/remix-run/react-router/blob/dev/packages/react-router-dom/index.tsx#L421-L424
81a92dc
to
f5602b7
Compare
<Link>
's "to" prop to accept absolute urls
952604a
to
40e0b6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments but looks good to me!
{shouldPrefetch && !isAbsolute ? ( | ||
<PrefetchPageLinks page={href} /> | ||
) : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this simplified approach 👌
… prefetch document external "to" detection should be brought over to React Router Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Co-authored-by: Matt Brophy <matt@brophy.org>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
4db32b3
to
7515d62
Compare
external "to" detection should be brought over to React Router (pr: remix-run/react-router#9900)
Signed-off-by: Logan McAnsh logan@mcan.sh
Closes: #5016
Testing Strategy:
our current testing for Link should be enough, both here and in RR