Skip to content

[chore] Router refactor #2655

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

Closed
wants to merge 6 commits into from
Closed

[chore] Router refactor #2655

wants to merge 6 commits into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Oct 20, 2021

This makes two primary changes (which are on two separate commits):

  • refactors out a Prefetcher separate from the Router
  • make the Router and Prefetcher use generic handling functions instead of depending on the Renderer (though not yet that generic since we'd need to cleanup the APIs)

This is a step towards making a separate @sveltejs/router project (#2611) by decoupling routing / rendering. The basic structure is now there though the interfaces would need to be cleaned up a fair amount

It's also possible this would be a step towards not pulling in prefetching code for people that aren't using it

No changeset since there are no user-visible changes

@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2021

⚠️ No Changeset found

Latest commit: 8415838

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benmccann benmccann changed the title Router refactor [chore] Router refactor Oct 21, 2021
trailing_slash,
handle_nav: renderer.handle_navigation.bind(renderer)
});
renderer.router = router;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible later on to resolve this somewhat cyclic dependency? I read this as "renderer depends on router, which depends on renderer"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's a good question. I'm not quite sure and don't have any ideas on how it would be done

the cycle isn't a new one. In master, the router takes the render as a constructor arg and then did this inside the constructor. I just moved where it's occurring. I agree with you that I don't love it, but am not sure how to change it. I'd be all ears if you have suggestions

Copy link
Member Author

@benmccann benmccann Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug into this a bit and added a comment explaining options for cleaning it up. It'd be too big of a change to try to tackle in this PR, but might make a nice follow up if I get some time 😄

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we can address the comment later if this provides a stepping stone to modularize the router.

@benmccann
Copy link
Member Author

I'm going to put this on ice temporarily. I've been working with the Routify folks to figure out how to make the routers swapable and I'm better figuring out through that process where it might be nice to draw some of the interface lines. This may still be a nice change, but possibly doesn't advance us towards making the routers swappable. I may reopen later, but right now want to dig into Routify more first because I think they've done a couple things that are pretty cool in terms of how they've structured things

@benmccann benmccann closed this Oct 22, 2021
@Conduitry Conduitry deleted the router-refactor branch March 5, 2022 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants