solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.1k stars 137 forks source link

Re-Export RouterContext for more control #413

Open iamcsharper opened 2 months ago

iamcsharper commented 2 months ago

The problem

I develop a PWA mobile-first application based on solid start.

I liked the prefetch behaviour while debugging the app in my laptop environment, so when you hover any link, the resources of that route are being downloaded, so no FUOC happens when you click the link.

However, when I tested the app on my phone, I noticed the flash of unstyled content instantly.

First approach

As first approach after looking through the code of solid router, I tried to emulate mouseover event on page load, for all the links found onMount. However, I see it as a big mistake

Proposed solution

I recommend to re-export the context, as it has all the neccessary data and methods to programmatically preload routes, instead of making another bad solution.

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: 8f0a8c322ea228c908932ef0419a2bf52156ab72

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | --------------- | ----- | | @solidjs/router | Minor |

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

ryansolid commented 3 weeks ago

Thanks. I agree this functionality is needed but I need to decide on the exposed API. To that end since this request is for preloading.. I've worked on making the API more extensible for the future and feel comfortable exporting it now. There will be a new method in the next release.

andreiborza commented 3 weeks ago

Having access to the router context would make it easier for third party libraries to interact with the router as well. In Sentry's case, we export a HOC to wrap Router just to use useBeforeLeave and useLocation. It would be nice if we didn't have to do that and users could just pass us the router context in, with a way to subscribe to route changes.

ryansolid commented 3 weeks ago

Yeah I get that. The problem right now is none of the APIs here are designed particularly well to be public facing. Like the implementation of useBeforeLeave:

export const useBeforeLeave = (listener: (e: BeforeLeaveEventArgs) => void) => {
  const s = useRouter().beforeLeave.subscribe({
    listener,
    location: useLocation(),
    navigate: useNavigate()
  });
  onCleanup(s);
};

It's using other router properties in its subscribe method which is weird since it could be encapsulated. I haven't really had the chance to evaluate all of these. Not to mention any other future extensions.

One thing is for certain the moment we expose this, we are on the hook for the internal API.