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

Add parameter route string to routes object. #7469

Closed
wants to merge 12 commits into from

Conversation

dennemark
Copy link
Contributor

@dennemark dennemark commented Jan 27, 2023

Adds a property path to routes object.
Route can still be called via routes.myRoute({ id: 1 }) giving an exact string like /myRoute/1. Alternatively we can get the parameter string via routes.myRoute.path: /myRoute/{id: Int}.

Also documents use of routes within useMatch.
Now it is convenient to match a given parametric route: useMatch(routes.myRoute.path).

Alternative would be, to call routes.myRoute() without any parameters, to get the original path. Not sure what is preferable.

The approach has one limitation though. If we have two routes with similar parameters, useMatch will be true for both of them. I added tests, too, but commented them, since they would fail for what we expect.

<Route path="/{dynamic}/{path}" page={MyPage} name="home" />
<Route path="/{another}/{path}" page={MyPage} name="anotherHome" />

Hope it suits redwood well!

@Tobbe
Copy link
Member

Tobbe commented Jan 27, 2023

Thanks for the PR! I'm always excited for new router related features 🙂
Will have to take a close look later.

In the meantime, could you give a more detailed use case for this? How/where/why would you be using it?

@dennemark
Copy link
Contributor Author

dennemark commented Jan 27, 2023

Specifically, I have this navbar, with four dynamic routes.
image
Each of them takes the same project parameter and its quite in the beginning of the url. So I do something like:

const Navbar () => {
  const { project } = useParams()
  const modes = [
    {
      name: "Info",
      route: routes.info({ project }),
      match: useMatch("/{project}/info").match,
    },
    {
      name: "Compare",
      route: routes.compare({ project, id: "1" }),
      match: useMatch(routes.compare.path).match,
    },
    // ...
  ]

  return (
    <>
      {modes.map((x) => <Button as={Link} to={x.route} isActive={x.match} />)}
    </>
  )
}

You can see in the example two different versions of match. I prefer the second one, since I use the same routes object and don't need to keep <Route path="/..."> and useMatch("/...") in sync.

@Tobbe
Copy link
Member

Tobbe commented Jan 27, 2023

Vielen Dank für das Beispiel.
"dont need to keep and useMatch("/...") in sync." This is a really strong reason for doing this. I'm sold 😃 Let's try to get this merged.

This adds to the public "api" of Redwood, so I want to get the team's input on syntax and stuff. I'll try to ask next week

@Tobbe
Copy link
Member

Tobbe commented Feb 7, 2023

@dennemark We talked about this one on our Core team meeting.
People didn't love routes.myRoute.path. It felt strange to access a property on a method like that. It's not something we do in the framework in other places.

We all agreed your use case makes sense though. Just wondering if there is another way we could help you solve it. Can we give you better tools to make this easier for you? So you don't manually have to compare raw strings.

But even if we do write a better/another useMatch or something like that, getting the raw path from the route is a reasonable ask. Another thing people have asked for before is "How can I get the name of the current route I'm on?". Could we come up with something that solves both those feature requests?

@dennemark
Copy link
Contributor Author

Would this be an alternative? Call routes.myRoute() without any parameters, to get the original path. Or write routes.myRoute(true) or something like it?
I don´t think something like routes.myRoute({originalPath: true}) would be good.
If you decide on a preference, I am up for implementing it!

@Tobbe
Copy link
Member

Tobbe commented Feb 14, 2023

@dennemark We discussed this PR again today. Currently leaning towards this syntax. Please let us know what you think

<Route path="/{dynamic}/{path}" page={ExamplePage} name="example" />

routes.example({ dynamic: 'dog', path: Param.ANY })
// => '/dog/{path}'

routes.example({ dynamic: Param.ANY, path: Param.ANY })
// => '/{dynamic}/{path}'

(Param would be a new object created just for the ANY constant)

Another possibility is routes.example({ dynamic: routes.ANY, path: routes.ANY }). We're also not super happy with ANY, but couldn't come up with anything better right now.

@jtoar
Copy link
Contributor

jtoar commented May 10, 2023

Per conversation with @thedavidprice, @Tobbe and I will take lead on the design decisions for this one. @dennemark are you still interested in working on this? Please see Tobbe's last comment above

@jtoar jtoar requested review from Tobbe and removed request for Tobbe May 10, 2023 17:59
@jtoar jtoar assigned Tobbe and jtoar May 10, 2023
@dennemark
Copy link
Contributor Author

dennemark commented May 15, 2023

@jtoar you can count me in.
So far we have these options:

  1.  routes.example({ dynamic: 'dog', path: Param.ANY })
     // => '/dog/{path}'
    
    • ➕ mix of dynamic and defined paths
    • ➖ new variable Param needs to be imported

    I can't think of a better way to avoid the import of Param, since I guess booleans are taken already for routing.
    Or is it possible to place param.Any somehow on the routes object?
    routes.example({dynamic: 'dog', path: routes.ParamAny})

  2. routes.myRoute.path: /myRoute/{id: Int}

    • ➕ fast to reach, since no import is needed?
    • ➖ only full dynamic path
    • ➖ mixture of function and variable on myRoute object
  3. optional parameters?
    3.1 routes.example({dynamic: 'dog'}, {path: "dynamic"})
    3.2 routes.example({dynamic: 'dog'}, ["path"])

    • ➕ no additional import
    • ➖ typing might be inconvenient?

@Tobbe
Copy link
Member

Tobbe commented May 15, 2023

@jtoar you can count me in.

Awesome! 🙂

optional parameters?
3.1 routes.example({dynamic: 'dog'}, {path: "dynamic"})
3.2 routes.example({dynamic: 'dog'}, ["path"])

  • no additional import
  • typing might be inconvenient?

Adding a second parameter to the function was also considered in #8069 for adding #<some-anchor> to the end of the generated URL. Now, it doesn't sound like we're going to do it that way, so no conflict. But wanted to mention it as semi-related. It was also voted down as bad DX, which we might want to consider as input here as well.

@dennemark
Copy link
Contributor Author

dennemark commented May 15, 2023

So we stick to variant 1
but need to decide on
1.1

import { Param, routes } from ...

routes.example({dynamic: 'dog', path: Param.ANY})

or
1.2

import { routes } from ...

routes.example({dynamic: 'dog', path: routes.ParamAny})

right?

@Tobbe
Copy link
Member

Tobbe commented May 15, 2023

@jtoar and I have a meeting scheduled later today. I'll make sure we discuss this PR.

Another option I just came up with, that might be more clear than "any" is Param.AsDefined or routes.AsDefined. As in "The parameter as it's defined in Routes.tsx", e.g. {id:Int} or {project}. It's more clear what it is, but probably less clear how it's supposed to be used...

@jtoar
Copy link
Contributor

jtoar commented May 16, 2023

Hey @dennemark, @Tobbe and I settled on RouteParams.LITERAL:

import { routes, RouteParams } from `@redwoodjs/router

routes.example({ dynamicParameter: 'dog', literalParameter: RouteParams.LITERAL })

We don't want to attach any more properties to routes because they could conflict with the dynamic properties on routes. (While it's unlikely anyone names a route LITERAL or some variant, we've run into naming collisions in other API surfaces we didn't expect before.)

I'm not sure if you need a Symbol for RouteParams.LITERAL. Maybe not, but it'd be a cool use case. Does this all sound good? And @Tobbe did I get this all right?

@Tobbe
Copy link
Member

Tobbe commented May 16, 2023

Yes, looks good to me 🙂

@jtoar jtoar added the release:feature This PR introduces a new feature label May 16, 2023
@jtoar
Copy link
Contributor

jtoar commented Jun 19, 2023

Hey @dennemark, were you still interested in this one? No pressure, just didn't want it to get lost

@dennemark
Copy link
Contributor Author

Hey @dennemark, were you still interested in this one? No pressure, just didn't want it to get lost

Hey! Yes i still have it on my list. But these days its not easy to find time...

@Tobbe
Copy link
Member

Tobbe commented Jun 19, 2023

@dennemark Thanks for checking in. No rush on getting it done. But do let us know if you want us to finish it up for you

@dennemark
Copy link
Contributor Author

@Tobbe @jtoar can you have a look at the latest commit? Test works out. Hope it is how you imagine it. I am just not sure if RouteParams.LITERAL is exported properly so it will be visible in the package later on.

@Tobbe
Copy link
Member

Tobbe commented Jul 28, 2023

@dennemark Thanks for sticking with this one 🙂

I rewrote your test a little bit, and added a few new test cases to the test. One of the new ones is failing. I'll discuss it with Dom on Monday.

Tobbe and others added 2 commits December 26, 2023 07:26
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@npmcli/arborist](https://togithub.com/npm/cli)
([source](https://togithub.com/npm/cli/tree/HEAD/workspaces/arborist)) |
[`6.5.0` ->
`7.2.2`](https://renovatebot.com/diffs/npm/@npmcli%2farborist/6.5.0/7.2.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@npmcli%2farborist/7.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@npmcli%2farborist/7.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@npmcli%2farborist/6.5.0/7.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@npmcli%2farborist/6.5.0/7.2.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

<details>
<summary>npm/cli (@&redwoodjs#8203;npmcli/arborist)</summary>

[`v7.2.2`](https://togithub.com/npm/cli/blob/HEAD/workspaces/arborist/CHANGELOG.md#722-2023-12-06)

-
[`ae2d982`](https://togithub.com/npm/cli/commit/ae2d98292472897b8365829633cd47a6cb006d03)
[#&redwoodjs#8203;7027](https://togithub.com/npm/cli/pull/7027) arborist:
`node.target` can be `null` when it is a file dep or symlink
([#&redwoodjs#8203;7027](https://togithub.com/npm/cli/issues/7027))
([@&redwoodjs#8203;ljharb](https://togithub.com/ljharb),
[@&redwoodjs#8203;lukekarrys](https://togithub.com/lukekarrys))
-
[`f875caa`](https://togithub.com/npm/cli/commit/f875caa86900122819311dd77cde01c700fd1817)
[#&redwoodjs#8203;6998](https://togithub.com/npm/cli/pull/6998) clean up
shrinkwrap code
([#&redwoodjs#8203;6998](https://togithub.com/npm/cli/issues/6998))
([@&redwoodjs#8203;wraithgar](https://togithub.com/wraithgar))

-
[`f656b66`](https://togithub.com/npm/cli/commit/f656b669e549286844f2071b9b62cf23f7958034)
[#&redwoodjs#8203;7062](https://togithub.com/npm/cli/pull/7062)
`@npmcli/[email protected]`
([#&redwoodjs#8203;7062](https://togithub.com/npm/cli/issues/7062))
([@&redwoodjs#8203;lukekarrys](https://togithub.com/lukekarrys))
-
[`9754b17`](https://togithub.com/npm/cli/commit/9754b173de26f3173e7f41eab34733fe9ba50f1d)
[#&redwoodjs#8203;7051](https://togithub.com/npm/cli/pull/7051) use global npm
for workspace tests
([@&redwoodjs#8203;lukekarrys](https://togithub.com/lukekarrys))
-
[`3891757`](https://togithub.com/npm/cli/commit/3891757f54d6d960cbf5f0d93d183d6424e8bed6)
[#&redwoodjs#8203;7051](https://togithub.com/npm/cli/pull/7051)
`@npmcli/[email protected]`
([@&redwoodjs#8203;lukekarrys](https://togithub.com/lukekarrys))

[`v7.2.1`](https://togithub.com/npm/cli/blob/HEAD/workspaces/arborist/CHANGELOG.md#721-2023-10-31)

[Compare
Source](https://togithub.com/npm/cli/compare/v7.2.0...84bf72e596eb8e8dcf62b2e508da3646828a5221)

-
[`dfb6298`](https://togithub.com/npm/cli/commit/dfb6298c3eb9fb7ef452906765ac5f23ea6fec49)
[#&redwoodjs#8203;6937](https://togithub.com/npm/cli/pull/6937) `[email protected]`
([#&redwoodjs#8203;6937](https://togithub.com/npm/cli/issues/6937))

[`v7.2.0`](https://togithub.com/npm/cli/blob/HEAD/workspaces/arborist/CHANGELOG.md#720-2023-10-02)

[Compare Source](https://togithub.com/npm/cli/compare/v7.1.0...v7.2.0)

-
[`81a460f`](https://togithub.com/npm/cli/commit/81a460f6e6317aca2288d16cda591aa6541540c6)
[#&redwoodjs#8203;6732](https://togithub.com/npm/cli/pull/6732) add
package-lock-only mode to npm query
([@&redwoodjs#8203;wraithgar](https://togithub.com/wraithgar))
-
[`0d29855`](https://togithub.com/npm/cli/commit/0d2985535c9cc3dfc3e1f355580570c9cce37d61)
[#&redwoodjs#8203;6732](https://togithub.com/npm/cli/pull/6732) add
no-package-lock mode to npm audit
([@&redwoodjs#8203;wraithgar](https://togithub.com/wraithgar))

-
[`0860159`](https://togithub.com/npm/cli/commit/0860159e18aa0fa985ef53fcfe0a57fbda995efb)
[#&redwoodjs#8203;6829](https://togithub.com/npm/cli/pull/6829) ensure workspace
links query parents correctly
([#&redwoodjs#8203;6829](https://togithub.com/npm/cli/issues/6829))
([@&redwoodjs#8203;Carl-Foster](https://togithub.com/Carl-Foster))
-
[`bef7481`](https://togithub.com/npm/cli/commit/bef7481282f18f5b8ad864dc76669801187029fe)
[#&redwoodjs#8203;6782](https://togithub.com/npm/cli/pull/6782) query with
workspace descendents
([#&redwoodjs#8203;6782](https://togithub.com/npm/cli/issues/6782))
([@&redwoodjs#8203;bdehamer](https://togithub.com/bdehamer))

-
[`aa6728b`](https://togithub.com/npm/cli/commit/aa6728b1d003f0fc620b074ba0396a3e07f2db6a)
[#&redwoodjs#8203;6859](https://togithub.com/npm/cli/pull/6859) `[email protected]`
-
[`ce9089f`](https://togithub.com/npm/cli/commit/ce9089f604a01297d3d2dd544283696a6297dce5)
[#&redwoodjs#8203;6859](https://togithub.com/npm/cli/pull/6859)
`[email protected]`
-
[`0a47af5`](https://togithub.com/npm/cli/commit/0a47af509d66071908c7e0bf065dcf2f4c877669)
[#&redwoodjs#8203;6859](https://togithub.com/npm/cli/pull/6859)
`[email protected]`
-
[`3ebc474`](https://togithub.com/npm/cli/commit/3ebc4744433d906e5c491d183fc077ffe79958cf)
[#&redwoodjs#8203;6859](https://togithub.com/npm/cli/pull/6859)
`@npmcli/[email protected]`

[`v7.1.0`](https://togithub.com/npm/cli/blob/HEAD/workspaces/arborist/CHANGELOG.md#710-2023-09-08)

[Compare Source](https://togithub.com/npm/cli/compare/v7.0.0...v7.1.0)

-
[`1c93c44`](https://togithub.com/npm/cli/commit/1c93c4430300e3b3bd2cb5bab327c1732f470bca)
[#&redwoodjs#8203;6755](https://togithub.com/npm/cli/pull/6755) Add `--cpu` and
`--os` option to override platform specific install
([#&redwoodjs#8203;6755](https://togithub.com/npm/cli/issues/6755))
([@&redwoodjs#8203;yukukotani](https://togithub.com/yukukotani))

[`v7.0.0`](https://togithub.com/npm/cli/blob/HEAD/workspaces/arborist/CHANGELOG.md#700-2023-08-31)

[Compare Source](https://togithub.com/npm/cli/compare/v6.5.0...v7.0.0)

-
[`fb31c7e`](https://togithub.com/npm/cli/commit/fb31c7e5f00ae39e67f9a5d6b6860c1d839c704b)
trigger release process
([@&redwoodjs#8203;lukekarrys](https://togithub.com/lukekarrys))

</details>

---

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44Ny4yIiwidXBkYXRlZEluVmVyIjoiMzcuODcuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@Tobbe Tobbe mentioned this pull request Dec 28, 2023
5 tasks
@Tobbe
Copy link
Member

Tobbe commented Dec 28, 2023

@dennemark I took another approach to how we could handle your use case. Let me know what you think! 🙂

See here: #9774

@Tobbe
Copy link
Member

Tobbe commented Jan 3, 2024

Closing this one in favor of #9793

@Tobbe Tobbe closed this Jan 3, 2024
jtoar pushed a commit that referenced this pull request Jan 3, 2024
Make it possible to specify route param values that need to match.

If this is your route: `<Route path="/blog/{year}/{month}/{day}"
page={BlogPostPage} name="blogPost" />`
And you want to only match posts from 2001 you can now do this:

`useMatch('/blog/{year}/{month}/{day}', { routeParams: { year: '2001' }
})`

This is **finally** a solution to matching route paths. The work started
in #7469, but we were never able to come up with an api/dx that we
really liked. This PR and #9755 together however provides a solution
that we're much more happy with, and that also supports the use case
outlined in that original PR.

Here's the example from #7469 as it could be solved with the code in
this PR

```jsx
const Navbar () => {
  const { project } = useParams()
  const routePaths = useRoutePaths()

  const modes = [
    {
      name: "Info",
      route: routes.info({ project }),
      match: useMatch(routePaths.info), // using the hook together with routePaths
    },
    {
      name: "Compare",
      route: routes.compare({ project, id: "1" }),
      match: useMatch(useRoutePath('compare')), // alternative to the above
    },
    // ...
  ]

  return (
    <>
      {modes.map((x) => <Button as={Link} to={x.route} isActive={x.match} />)}
    </>
  )
}
```

And, as described at the top of this description, we can also be more
specific than in that example if needed. Like if we only wanted to match
a specific project on the "Compare" route we could do this:

```jsx
  const modes = [
    {
      name: "Info",
      route: routes.info({ project }),
      match: useMatch(routePaths.info),
    },
    {
      name: "Compare against Alpha",
      route: routes.compare({ project, id: "1" }),
      match: useMatch(useRoutePath('compare'), { routeParams: { project: 'alpha' } }),
    },
    {
      name: "Compare against Beta",
      route: routes.compare({ project, id: "1" }),
      match: useMatch(useRoutePath('compare'), { routeParams: { project: 'beta' } }),
    },
    // ...
  ]
```

Here's another example

```jsx
<Route path="/{dynamic}/{path}" page={ExamplePage} name="example" />

const exampleRoutePath = useRoutePath('example')
// => '/{dynamic}/{path}'

const matchOnlyDog = useMatch(exampleRoutePath, { routeParams: { dynamic: 'dog' }})
const matchFullyDynamic = useMatch(exampleRoutePath)
```

In the above example, if the current page url was
`https://example.org/dog/fido` then both `matchOnlyDog` and
`matchFullyDynamic` would have `match: true`.
If the current page instead was `https://example.org/cat/garfield` then
only `matchFullyDynamic` would match

(This PR replaces #9774)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants