Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Abstract the router requirement #858

Open
RobinKnipe opened this issue Dec 11, 2023 · 8 comments
Open

Abstract the router requirement #858

RobinKnipe opened this issue Dec 11, 2023 · 8 comments

Comments

@RobinKnipe
Copy link
Collaborator

Currently there is a hard dependency on react-router which means the project can't be used in other routing scenarios (e.g. Next.JS). By abstracting the routing, the project could be made to work with other SSR environments.

@daniel-ac-martin
Copy link
Owner

@RobinKnipe: Any ideas on how we can do this in practice? - I'd rather not drop support for react-router.

BTW, how does Next.js do this? Does it magically intercept normal <a> links rather than using a React component? Or do we need to find a way to switch to Next's version of <Link>?

@BenJenkinson
Copy link

Next.js has its own <Link> component which renders as an <a> tag

📚 https://nextjs.org/docs/app/api-reference/components/link

It's used in much the same way

<Link href="/dashboard">Dashboard</Link>
// Navigate to /about?name=test
<Link
  href={{
    pathname: '/about',
    query: { name: 'test' },
  }}
>
  About
</Link>

It does have some differences though; it is intended only for internal links and throws a warning if used for external ones.

Invalid hrefs include external sites (https://google.com) and mailto: links. In the past, usage of these invalid hrefs could have gone unnoticed, but since they can cause unexpected behavior we now show a warning in development for them.

📚 https://github.com/vercel/next.js/blob/canary/errors/invalid-href-passed.mdx

@daniel-ac-martin
Copy link
Owner

In that case, it's the same as it's always been.

I suppose the question is can we detect the context we are in? i.e. A react-router or a next-router? (Perhaps via a literal React 'Context'?) If we can do that, we can simply switch between the relevant upstream Link components in our own Link component.

@RobinKnipe
Copy link
Collaborator Author

I believe Next has a build phase - it attempts to do SSG and the necessary processing for asset creation (CSS/JS/etc) - so you would know then whether the context were Next or not.

@daniel-ac-martin
Copy link
Owner

@daniel-ac-martin
Copy link
Owner

@SamChatfield
Copy link

SamChatfield commented Oct 12, 2024

Another possibility for achieving this could be using React Context. I'm imagining something like a NotGovUkRouterContext.Provider which could allow you to set the link component to use either globally or specifically overriding for things like the Header, Footer, BackLink etc., as well as configure any other router behaviour. NotGovUk components that render links would then read and evaluate this context to pick the link component to use, or fall back to a plain anchor tag if none was provided.

This could be simpler implementation-wise and more configurable for consumers of this library because you may not want every link in the application to be handled by React Router or Next.js. This also means you don't need to require either react-router or Next.js as a dependency at all, you just let the library consumer pass you the component they want you to render.

@daniel-ac-martin
Copy link
Owner

Hi @SamChatfield.

That's an interesting idea, thanks. If I'd thought of that a month or so ago, I probably would have tried to go with something like that as a first version.

There are some complications, in that the Link components that are provided by Next.js and react-router have different APIs. We also, don't only interact with the router via the Link component, but rather make use of their hooks. (e.g. useLocation and useNavigate.) One example of this is to determine whether a link is currently 'active'; annoyingly to perform this operation in the same way on Next.js requires a suspense boundary.

That said, it may be possible to have some dedicated packages for each framework. (e.g. @not-govuk/next-compat) and have the users pass it into the Context.Provider that you propose.

Anyway, I think I'm pretty close with this. Current status is here: #1039 (comment)

My approach has been to re-purpose the @not-govuk/route-utils package to provide a react-router compatible interface (well, as subset at least) and to support both react-router and next's (app) router by detecting whether the necessary packages are available. (Which is don't via a bit of a dirty hack, but there you are.)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants