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

[RFC] useLink #7329

Closed
timneutkens opened this issue May 14, 2019 · 20 comments
Closed

[RFC] useLink #7329

timneutkens opened this issue May 14, 2019 · 20 comments

Comments

@timneutkens
Copy link
Member

timneutkens commented May 14, 2019

Goals

  • Have a dependency graph of pages that depend on each other at build time for smart bundling optimizations
  • Smaller runtime for linking to other pages
  • Getting rid of cloneElement
  • Easier to understand API, no more href vs as
  • Being able to lint for links that we know at build time will always 404
  • Replace next/link but retaining backwards compatibility with it.
  • Easier to understand dynamic routing, as there's a distinction between the page being rendered and the url.
  • Get rid of buildId in the page file url to improve long-term caching.

API

Simple usage:

// pages/index.js
import { useLink } from 'next/link'

function HomePage() {
  const AboutLink = useLink('/about', () => import('./about'))
  return <>
    Go to about page
    <AboutLink>About</AboutLink>
  </>
}

So what does useLink return?

  • An enhanced <a> component with href , onClick and ref set

  • Automatically prefetches based on viewport

  • import() allows linters to throw when the file doesn't exist (TS, eslint etc)

  • You no longer have to write <a> inside of <Link>

Alternatives

An alternative API that involves code-generation and is harder to type using TypeScript would be:

// pages/index.js
import { A as AboutLink } from './about'

function HomePage() {
  return <>
    Go to about page
    <AboutLink>About</AboutLink>
  </>
}

It would give the same linting / 404 detection benefits as the import API, we'd abstract it away from the user using code generation.


One issue that was raised by @lydiahallie is that creating a navbar would look like this:

const HomeLink = useLink('/', () => import('./index'))
const BlogLink = useLink('/blog', () => import('./blog'))
const BlogHelloWorldLink = useLink('/blog/hello-world', () => import('./blog/hello-world'))
const SupportLink = useLink('/support', () => import('./support'))
const SomethingElseLink = useLink('/something-else', () => import('./something-else'))
const AboutLink = useLink('/about', () => import('./about')) // Copied as filler to give same effect
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))
const AboutLink = useLink('/about', () => import('./about'))

The solution for doing something like that might be using returned hook value:

const links = [
  useLink('/', () => import('./index')),
  useLink('/blog', () => import('./blog')),
  useLink('/blog/hello-world', () => import('./blog/hello-world')),
  useLink('/support', () => import('./support')),
  useLink('/something-else', () => import('./something-else')),
  // etc
]
@timneutkens
Copy link
Member Author

timneutkens commented May 14, 2019

This solves #5707 and #6303
Also probably solves #2581

@Janpot
Copy link
Contributor

Janpot commented May 14, 2019

Haven't thought deeply about this yet, but does an additional programmatic API for this make sense?

// pages/index.js
import { useRoute } from 'next/router'

function HomePage() {
  const { push, replace } = useRoute('/about', () => import('./about'))
  return <>
    Go to about page
    <button onClick={push}>About</button>
    <button onClick={replace}>About (replace)</button>
  </>
}

@timneutkens
Copy link
Member Author

timneutkens commented May 14, 2019

It wouldn't because at that point we can't do prefetching automatically based on the viewport which is what we currently do for next/link.

Note that next/router will accept a React component in push / replace, which will allow you to write that useRoute implementation if you'd like.

@Janpot
Copy link
Contributor

Janpot commented May 14, 2019

Makes sense. I was more thinking about the linting part, but that's probably not a big enough win to increase the API surface by so much.

@timneutkens
Copy link
Member Author

timneutkens commented May 14, 2019

import() is what would automatically because the build/linting part of the solution. So it'd work with next/router.

@Janpot
Copy link
Contributor

Janpot commented May 14, 2019

Oh right, now I see. Thanks, that's a very elegant API 👍

@jhoffmcd
Copy link
Contributor

jhoffmcd commented May 14, 2019

So would users be able to change the route string but not the page component file name in this new API? Or do the route name and file name need to match?

@tricoder42
Copy link

tricoder42 commented May 14, 2019

Hey @timneutkens, thank you for writing this up. Interesting approach.

Few questions though:

You no longer have to write <a> inside of <Link>

  1. How would I style the link or use styled component with this hook?

  2. How would you pass route parameters? e.g. /article/:id?

  3. As with previous link, I don't like the ad-hoc route configuration. It's very common that the link is reused on multiple pages and if I had to copy the cofiguration to each page, I'm just asking for troubles. But I guess I could create a file with custom links:

// links.ts
export const useAboutLink = () => useLink('/about', () => import('./about'))
export const useArticleLink = () => useLink('/article', () => import('./article'))
// pages/index.ts

import { useAboutLink } from '../links'

export default () => {
  const AboutLink = useAboutLink()

  return (
    <nav>
       <AboutLink>About</AboutLink>
    </nav>
  )
}

I use similar approach with current Link and so far it works fine. It's actually "one line easier", because I can import the link directly. Thanks to tree-shaking, the bundle should always have just the links included on page and not the full route config.

  1. And final question, this is only for client, right? On the server I still have to add a custom handler.

Thanks for the hard work 🙏

@baer
Copy link

baer commented May 14, 2019

Is this the proposal to deal with dynamic routes that you were referring to in your comment on the thread on Dynamic Routing and in the RFC on API routes? I noticed that you didn't list #4989 as a closable issue in your comment above.

@anthonyshort
Copy link
Contributor

I love the idea of using a hook API for links. One of the most awkward APIs in Next is the way the links work. They are:

  • Hard to style. If you're using something like styled components or wanting to link using a button, you end up needing to drop down to using the router directly.
  • Magic: The way that it clones the element and injects the href is confusing. Especially for new people. Having links in the tree without a href can make linters complain.

I'd suggest that the goal of the link API should be:

  • Explicit. Returning handlers enables the end-user to then create their own link component that might be styled using whatever CSS system they've already got setup.
  • Consistent: We should be able to use the same API when we're trying to do any routing/linking with an app.

For example, in use-next-route I used this API:

const { href, onClick, navigate } = useLink('/projects')

This lets me decouple the styled links from next functionality. My projects will usually have a set of low-level UI components. Let's say I'm using styled-components:

const { href, onClick, navigate } = useLink('/projects')

return (
  <StyledLink href={href} onClick={onClick} />
)

or if I want a button click to trigger a navigation, the API stays the same:

const { href, onClick, navigate } = useLink('/projects')

return (
  <button onClick={onClick}>Go there</button>
)

or if I want to navigate on a form submission:

const { href, onClick, navigate } = useLink('/projects')

function onSubmit() {
  // handle the form
  navigate()
} 

return (
  <form onSubmit={navigate}>...</form>
)

There's also the problem wanting to pass in parameters. You can do this when you create the link, just like in Router.push:

const projectRoute = useRoute({
    pathname: '/project/details',
    query: {
      id: props.project.id
    }
})

or you can pass in params when you call navigate, for times when you don't know what the parameters are during render:

const { navigate } = useLink()

function onSubmit() {
  navigate({
    pathname: '/project/details',
    query: {
      id: props.project.id
    }
  })
}

One benefit of doing it this way is you can pull out all of your routes into another file, so you can re-use the logic/parameters, regardless of what type of element is trying to trigger the navigation. In my apps, I have a hooks/routes.ts file that contains all of the routes that any component can use:

export function useProjectRoute(projectId: string) {
  return useRoute({
    pathname: '/project/details',
    query: {
      id: projectId
    }
  })
}

If we were to return an anchor element it limits the ways the route could be used. Let's say we tried to follow a similar pattern:

export function useProjectRoute(projectId: string) {
  return useLink('/projects/details', () => import('./about')) 
}

This returns an <a>, but I still want to pass in the projectId and I might want to trigger this route after a form submission or on a button click.

I'm not too sure how this might affect the ability to create a dependency graph, but maybe we could have a combination of the two:

  return { href, onClick, navigate } = useLink('/project/details', () => import('/project/details'))

with the option to pass in parameters:

return { href, onClick, navigate } = useLink({
  pathname: '/project/details'
}, () => import('/project/details'))

But it looks a little awkward. I'm not too sure about the mechanics of the import so I think if I understood that a little more I could suggest something better :)

@damusnet
Copy link
Contributor

Hey, this sounds very promising (really love all your RFCs lately!).

In all the projects I've worked on we've always rather used Router.push() rather than the <Link /> component because we always end up having to abstract some common logic. (Also, having <a> inside <Link> is weird and throws linters off.)

Some examples I can think of:

  • sending an analytics event
  • scrolling back up to the top of the window after navigation
  • closing a menu or updating some other state
  • formatting urls

The last one is probably the biggest one. I mentioned in the other RFC that we serve everything under an asset prefix /new. Our custom routerPush() function that wraps Router.push() is where we can push the correct page (say pathname: /user for instance), pass the query params (query: { id: 42 }), and format the vanity url (as: "/new/user/42/damien").

I guess all of this is probably possible with <Link /> and the Router.* events, but using a function just seems easier to read.

Would this proposal be able to still do prefetching if we wrap useLink and compose it like other hooks?

In any case I love the import part and honestly don't mind how verbose it is given the benefits!

@tricoder42
Copy link

Interesting 🤔 I actually like how Link works, that it only manages routing and rendering is left on the child component.

Hard to style. If you're using something like styled components or wanting to link using a button, you end up needing to drop down to using the router directly.

@anthonyshort I use Link component only with styled components without any problems. Alright, there's a one caveat that you need to add passHref attribute:

<Link href="/about" passHref>
  <StyledButton>About</StyledButton>
</Link>

But again, this solution is beautiful that Link takes care of routing and all Link props are related to routing, while StyleButton is reposible for rendering. Link component API can be upgraded without breaking the rendered props.

On the other hand, having one component to do both routing and rendering might look simpler but it's less flexible. It's harder to update Link API (add new prop) as now it might break any existing components which use the same prop. The props namespace might be a problem even when you want to reuse 3rd party component as a Link.

Magic: The way that it clones the element and injects the href is confusing. Especially for new people.

It isn't a magic, just not a pattern which you see very often, yet. Reach UI does amazing things with child components. It certainly makes the public API simpler. You could have an explicit render prop pattern:

<Link>{
 (linkProps) => <a {...linkProps}>About</a>
}</Link>

It's more explicit, but harder to write and read. React.cloneElement is a public API, why not use it if it makes life easier.

Having links in the tree without a href can make linters complain.

If the linter complains about a valid code, then it's broken and should be disabled or replaced with more appropriate one.

@jth0024
Copy link

jth0024 commented May 19, 2019

How would you manage rendering links to unknown routes with this API? For example, in our app we receive a list of menu links from an external CMS. Since our links are dynamic, this proposal wouldn't work for us. We wouldn’t know which file to import and we would have to use string interpolation in the import statements. From what I understand, interpolated import statements cause a lot more code to be shipped to the client than expected (Webpack has to bundle every possible match). Here's an example to illustrate the issue I'm describing:

// Topbar.js
function Topbar(props) {
  const { links } = this.props
  return (
    <header>
      {...links.map(link => (<TopbarLink {...link}/>))}
    <header/>
  )
}

// TopbarLink.js
import { useLink } from 'next/link'

function TopbarLink(props) {
  const { page, text } = props
  const Link = useLink(page, () => import(`./${page}`))
  return <>
    <Link>{text}</Link>
  </>
}

@tannerlinsley
Copy link

Hey all! This is my first comment to Next.js in a very very long time :) TL;DR: So freaking excited to be here. Hope I can help Next.js grow!

At first look, a few thing feel funny here:

  • For most use-cases now, LOC are essentially doubled with not a lot of added face value to the user (clearly this has massive benefits in the long run and to the framework though). It's a hard sell up front IMO
  • Dynamically generating links would stress the laws of hooks, since you would need to ensure that they are called every render in the same order. It seems like existence of a link isn't exactly idempotent or persistent like hooks normally are.

But, also getting some good vibes:

  • With a hook, you're essentially turning the link magic into headless props that you can use to build your own links with your own UI. At the very least, this is a good lower level option for people that need/want it, but I would still want to consume it as a component during usage.
  • The concept of prefetching based on a hook is a good idea. I designed a usePrefetch hook in React Static that would handle viewport prefetching in a very concise API. This reminds me of that a bit and people loved that feature over there.

This starts to feel like a lot of responsibility is being handed to the user where the framework should be able to handle the magic instead. Unfortunately though, I can't think of a better way to achieve the benefits in the OP.

@kylemh
Copy link
Contributor

kylemh commented Jul 17, 2019

@timneutkens how does this RFC interop w/ dynamic routes? Would love to see some examples in the OP.

My guess:

const BlogLink = useLink(`/blog/${blogID}`, () => import('./blog'))

@timneutkens
Copy link
Member Author

timneutkens commented Aug 10, 2019

I'm going to close this RFC as it has quite a few downsides as pointed out by Sebastian from the React team. We're working on improving the existing next/link component.

@damusnet
Copy link
Contributor

Hi @timneutkens that's too bad, it was very interesting! Are Sebastian's comment public to read somewhere or was it private or a conversation? Just curious to know more about the limits he identified.

@timneutkens
Copy link
Member Author

Private. Main limitation is returning a component from a hook, which causes some issues with memoization.

@damusnet
Copy link
Contributor

Alright, thanks, good to know!

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests