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

Re-Export RouterContext for more control #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iamcsharper
Copy link

@iamcsharper iamcsharper commented May 6, 2024

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.

Copy link

changeset-bot bot commented May 6, 2024

🦋 Changeset detected

Latest commit: 8f0a8c3

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
Copy link
Member

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
Copy link

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
Copy link
Member

ryansolid commented Jun 17, 2024

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.

@johndunderhill
Copy link

johndunderhill commented Sep 27, 2024

@ryansolid I sympathize with your concerns about exposing the API. However right now this is giving us fits, as we can see no reliable way to determine the current route, or access its metadata. This seems like very basic functionality. We use config-based routing, and have additional metadata in the route object that we desperately need to set up the page.

There are no good work-arounds that we can see. Looking up the route by path won't work. For one thing, we can't rely on the path at all until the route stabilizes (cf. useIsRouting()), which makes everything downstream of it async. Moreover, to match the path correctly, we'd need to duplicate the router's patturn matching algorithm. We do not see any documented routines we can call to help with that. We can't figure out how to use isMatch() for anything useful, and we don't understand what useLocation() returns during route transitions.

The router has useBeforeLeave(), but no useBeforeEnter(), so we can't hook the route transitions ourselves at the right moment. The latter would silence the many people crying out for hooks to implement centralized auth checks, a very real problem we are also struggling with, and which has required ugly work-arounds.

Moreover, we do not understand why all the 'primitives' return function generators, 'reactive objects' (whatever those are), or signals (ok, we do know what those are), and why some return one rather than the other. In many cases, we just need the value once, right now, whatever it is, without having to worry about callbacks, memos, or whether something will be called repeatedly later on, creating unintended effects. Do we use untrack here?

We don't want to hook into an undocumented API, but we're reaching a very high level of furstration here. Is there any hope of getting this sorted out any time soon?

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

Successfully merging this pull request may close these issues.

4 participants