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

Add previous/current route instances to RouterOnChangeArgs #389

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

Add previous/current route instances to RouterOnChangeArgs #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a2104a
Add previous/current route instances to RouterOnChangeArgs
peabnuts123 Oct 15, 2020
c1b3aa2
Merge branch 'master' into feature/previous-route-instance
peabnuts123 Nov 23, 2020
a1487db
Remove redundant variable alias
peabnuts123 Dec 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ export interface RouterOnChangeArgs {
router: Router;
url: string;
previous?: string;
previousRoute?: preact.VNode,
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on how you'd use this? Just curious.

Copy link
Author

Choose a reason for hiding this comment

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

This is already what current is. This PR just brings current/previous in parity with one-another. Currently previous is a string, while current is a Route instance. Not only is this inconsistent, it makes it hard / impossible to implement logic that uses props from the previous route.

I am depending on these changes in a project of mine, where I use this data to determine whether the user is navigating to/from the default route: use-case.

Copy link
Member

Choose a reason for hiding this comment

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

Ignoring the oddness of calling a browser refresh, why watch the props rather than the route?

Watching component props like this to me sounds like an anti-pattern. I'm not saying that this shouldn't get merged or anything but that there's almost certainly a better way to do what you want.

Copy link
Author

Choose a reason for hiding this comment

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

I'd love to find a work around. I need to know information about the previous route, but I cannot resolve this from the URL. I could manually build up a dictionary of this data myself, keyed by URL, and try and do this lookup myself (including mapping URLs to dictionary keys i.e. trimming query params, etc.), but really it is the responsibility of the Router to give me this information (especially, in my use-case above, whether a Route is the default route or not).

active: preact.VNode[];
current: preact.VNode;
currentRoute: preact.VNode;
}

export interface RouterProps extends RoutableProps {
Expand Down
6 changes: 5 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,19 @@ class Router extends Component {
let current = active[0] || null;

let previous = this.previousUrl;
let previousRoute = this.previousRoute;
if (url!==previous) {
this.previousUrl = url;
this.previousRoute = current;
if (typeof onChange==='function') {
onChange({
router: this,
url,
previous,
previousRoute,
active,
current
current,
currentRoute: current
rschristian marked this conversation as resolved.
Show resolved Hide resolved
});
}
}
Expand Down