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.

Sign up for GitHub

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

Jump to bottom

[DO NOT MERGE] Switch to @reach/router #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[DO NOT MERGE] Switch to @reach/router #90

wants to merge 1 commit into from

Conversation

ryanflorence
Copy link

@ryanflorence ryanflorence commented Aug 8, 2018 •
edited
Loading

Hey folks 👋🏼

I talked to @jxnblk and he expressed interest in switching to Reach Router (https://reach.tech/router). A few reasons to do it:

I was surprised how straightforward the migration was, only took an hour or so 😮

A couple todos:

  • get npm run build to work, (I couldn't get it to work on master either, something about the xo/ basename, so I think it's fine? need somebody else to confirm)
  • update documentation that references Link or any other router API

What do you think?

@ryanflorence
[WIP] Switch to @reach/router
f90d8cd 
- adds more accessible route transitions
- decreases bundle size
- supports relative links
@jxnblk
Copy link
Member

jxnblk commented Aug 8, 2018

This looks great, thanks!

get npm run build to work, (I couldn't get it to work on master either, something about the xo/ basename, so I think it's fine? need somebody else to confirm)

By get it to work, do you mean running the exported site in a local server? The basename is set up so that it works at https://compositor/x0 and it's fine to disable that flag for testing it out locally...

update documentation that references Link or any other router API

Yeah, I think it might be nice to hide the internals of routing a bit better, e.g. have a Link component that can be imported from this library. I would guess that switching out the router implementation here will be a major version change

@ryanflorence
Copy link
Author

The basename is set up so that it works at https://compositor/x0 and it's fine to disable that flag for testing it out locally

Yes, I got it to run by removing pkg.x0.basename, but I wanted to test out the basename handling too. I don't know how to get the basename in at build time on master or this branch.

@ryanflorence
Copy link
Author

ryanflorence commented Aug 8, 2018 •
edited
Loading

Yeah, I think it might be nice to hide the internals of routing a bit better,

If you'd like. I don't have an opinion either way. Tradeoff seems to be that now you have an API you have to maintain (so if somebody wants to render some router aware <Tabs/> things aren't as straightforward, you'll have to document it, add more code, make more decisions about what part of the API you copy, what you change, what you leave out, etc.), but upside is if you switch routers again it wouldn't be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanflorence @jxnblk