feat: handle external url in `Link` on native

nandorojo / solito

🧍‍♂️ React Native + Next.js, unified.

https://solito.dev

MIT License

3.54k stars 181 forks source link

feat: handle external url in `Link` on native #194

Closed intergalacticspacehighway closed 1 year ago

intergalacticspacehighway commented 2 years ago

Issue

NextJS's Link component works with external URLs. So, I think it makes sense to handle it on native as well.

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
solito-s9oj	✅ Ready (Inspect)	Visit Preview	Dec 9, 2022 at 6:50AM (UTC)
with-custom-fonts	✅ Ready (Inspect)	Visit Preview	Dec 9, 2022 at 6:50AM (UTC)

2 Ignored Deployments

nandorojo commented 2 years ago

My only question here is how we're detecting an external link. Is there a way to know how browsers do it? Is startsWith('/') correct?

For example, if you accidentally start with artists/drake, what should we do?

intergalacticspacehighway commented 2 years ago

if you accidentally start with artists/drake, what should we do?

yeah, I thought about it. Apparently, React Navigation requires path to start with /, or else it throws an error. I missed adding it in the description. Do you think it can still break?

https://github.com/react-navigation/react-navigation/blob/d7032ba8bb6ae24030a47f0724b61b561132fca6/packages/native/src/useLinkTo.tsx#L45

nandorojo commented 2 years ago

yeah but it will never error in this case because we don't forward the URL down. I'm wondering how the URL should be detected...

Should we check if it includes ://? This would allow http*:// as well as app deep links like beatgig://.

If we only match local requests for /*, then an empty "" string would not error from React Navigation (as it should) and will instead open a web browser on native. Meanwhile, an empty string would do nothing on Web.

There are edge cases, but just want to make sure we account for them properly to match expected behavior.

intergalacticspacehighway commented 2 years ago

Makes sense. I just took a look at how Next handles it. I think we could use the isAbsoluteUrl. It's doing a simple regex test to detect if URL is absolute which would make sense for external URLs. wdyt?

nandorojo commented 2 years ago

I think that makes sense!

intergalacticspacehighway commented 2 years ago

@nandorojo updated. Not sure why it's ignoring 2 deployments 🤔

nandorojo commented 2 years ago

Left some final minor comments then this looks good to me!