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

feat: Add absolute paths for routers using base #153

Merged
merged 1 commit into from
Dec 14, 2020
Merged

feat: Add absolute paths for routers using base #153

merged 1 commit into from
Dec 14, 2020

Conversation

jvdsande
Copy link
Contributor

@jvdsande jvdsande commented Dec 13, 2020 •
edited

This PR is linked to issue #152.

  • Adds support for 'absolute' paths: paths starting with the token ~ are always handled as absolute paths, even inside a Router using base
  • Links correctly set the href attribute accordingly
  • useLocation returns location accordingly, thus making matcher work without any changes needed
  • Added tests for Route, Redirect, useLocation and Link

Taking the following sample:

<Router base="/app">
  <Link href="/relative">Relative Link</Link>
  <Link href="~/absolute">Absolute Link</Link>
</Router>

The first Link will point to /app/relative, while the second points to /absolute

Taking the following sample:

<Router base="/app">
  <Switch>
    <Route path="/subpage">Subpage</Route>
    <Route path="/">
	  <Redirect to="/subpage" />
    </Route>
  </Switch>
</Router>

Redirect will match /app and redirect to /app/subpage. It will not match / which will be reported as ~/ inside the Router.

@codecov
Copy link

codecov bot commented Dec 13, 2020 •
edited

Codecov Report

Merging #153 (fe1c2ef) into master (77e20ef) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #153   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          237       239    +2     
  Branches        48        50    +2     
=========================================
+ Hits           237       239    +2
Impacted Files Coverage Δ
use-location.js 100.00% <ø> (ø)
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77e20ef...fe1c2ef. Read the comment docs.

jvdsande
jvdsande commented Dec 13, 2020
View reviewed changes
README.md
@@ -67,7 +67,7 @@ Wouter provides a simple API that many developers and library authors appreciate
- [TypeScript support](#can-i-use-wouter-in-my-typescript-project)
- [Using with Preact](#preact-support)
- [Server-side Rendering (SSR)](#is-there-any-support-for-server-side-rendering-ssr)
- [Routing in less than 350B](#1kb-is-too-much-i-cant-afford-it)
- [Routing in less than 400B](#1kb-is-too-much-i-cant-afford-it)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated here since 350B was not true anymore (even before this PR). I've put 400 since it's still a nice goal, and it allows for a bit of changes since the size is now 380.

Copy link
Owner

Choose a reason for hiding this comment

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

Good one!

jvdsande
jvdsande commented Dec 13, 2020
View reviewed changes
README.md
@@ -332,7 +332,7 @@ Read more → [Customizing the location hook](#customizing-the-location-hook).
- **`matcher: (pattern: string, path: string) => [match: boolean, params: object]`** — a custom function used for matching the current location against the user-defined patterns like `/app/users/:id`. Should return a match result and an hash of extracted parameters. It should return `[false, null]` when there is no match.

- **`base: string`** — an optional setting that allows to specify a base path, such as `/app`. All application routes
will be relative to that path.
will be relative to that path. Prefixing a route with `~` will make it absolute, bypassing the base path.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I tried to be as succinct as possible to match the style of the ReadMe. I can elaborate a bit (maybe with samples?) as this might not be clear enough.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, definitely a good idea! I'm going to work on an example, already have some ideas for the demo.

molefrog
molefrog approved these changes Dec 14, 2020
View reviewed changes
Copy link
Owner

@molefrog molefrog left a comment

Choose a reason for hiding this comment

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

Fantastic work!

@molefrog molefrog merged commit 864a910 into molefrog:master Dec 14, 2020
@molefrog
Copy link
Owner

All looks great, I've merged into the master branch. The further steps are:

  • Working on some CodeSandbox examples to illustrate the multiple sub-apps use case and global routing.
  • Preparing for the release

@jvdsande
Copy link
Contributor Author

Awesome, thanks!

I can put together one or two CodeSandbox examples if you want. I was thinking something along the line of:

MyAppOne / MyAppTwo with two subpages each, with MyAppOne having a link to a subpage of MyAppTwo, and vice-versa.

@jvdsande jvdsande mentioned this pull request Dec 14, 2020
Closed
@jvdsande
Copy link
Contributor Author

jvdsande commented Dec 14, 2020 •
edited

@molefrog

Here is a first go I had with a sandbox:
https://codesandbox.io/embed/wouter-absolute-paths-ccovi

Of course it is not working right now, since it uses current wouter release.

@molefrog
Copy link
Owner

molefrog commented Dec 16, 2020 •
edited

@jvdsande I've just published 2.7.0-alpha.1, could you try to install this version in your project and check if it resolves the issue?

UPD: I forked the sandbox and it appears to be working!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@molefrog molefrog molefrog approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jvdsande @molefrog