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

(preact-iso) - support route parameters #300

Closed
wants to merge 23 commits into from
Closed

Conversation

JoviDeCroock
Copy link
Member

Resolves: #298

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2021

Size Change: +205 B (0%)

Total Size: 693 kB

Filename Size Change
demo/dist/chunks/compat.********.js 15.3 kB -1 B (0%)
demo/dist/chunks/index.********.js 190 B +1 B (+1%)
demo/dist/chunks/prerender.********.js 2.45 kB +1 B (0%)
demo/dist/error/index.html 623 B +1 B (0%)
demo/dist/index.********.js 6.92 kB +172 B (+3%)
demo/dist/index.html 713 B +31 B (+5%) 🔍
ℹ️ View Unchanged
Filename Size Change
demo/dist/about/index.html 636 B 0 B
demo/dist/assets/Calendar.********.css 702 B 0 B
demo/dist/assets/style.********.css 386 B 0 B
demo/dist/chunks/class-fields.********.js 201 B 0 B
demo/dist/class-fields/index.html 612 B 0 B
demo/dist/compat/index.html 1.48 kB 0 B
demo/dist/env/index.html 692 B 0 B
demo/dist/files/index.html 654 B 0 B
wmr.cjs 661 kB 0 B

compressed-size-action

@rschristian
Copy link
Member

Might be useful to add a route with parameters to the preact-iso ReadMe too.

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

Here's a few simplifications.

packages/preact-iso/router.js Outdated Show resolved Hide resolved
packages/preact-iso/router.js Outdated Show resolved Hide resolved
packages/preact-iso/router.js Outdated Show resolved Hide resolved
packages/preact-iso/router.js Outdated Show resolved Hide resolved
packages/preact-iso/router.js Outdated Show resolved Hide resolved
packages/preact-iso/router.js Outdated Show resolved Hide resolved
@smeijer
Copy link

smeijer commented Jan 12, 2021

I've been able to try this branch, and the routing part works great! Can't wait to see this being merged.

But... the useLocation hook does return these parameters. I think being able to retrieve and use these parameters, is what would make this truly useful.

@JoviDeCroock
Copy link
Member Author

@smeijer You mean like useLocation().parameters would contain all the :id, ....

@smeijer
Copy link

smeijer commented Jan 13, 2021

@smeijer You mean like useLocation().parameters would contain all the :id, ....

Exactly! Extending the example from the docs in this PR:

const App = () => (
	<LocationProvider>
		<ErrorBoundary>
			<Router>
				<Home path="/" />
				<Profiles path="/profiles" />
				<Profile path="/profiles/:id" />
				<Profile path="/profiles/:id/:tab" />
			</Router>
		</ErrorBoundary>
	</LocationProvider>
);

When on example.com/profiles/123, useLocation().params would be

{
  id: '123',
}

When on example.com/profiles/123/hobbies, useLocation().params would be

{
  id: '123',
  tab: 'hobbies',
}

That's matching behavior with react-router#useParams. Having a separate hook for it named useParams would ease migration for users of react + react-router.

@developit
Copy link
Member

developit commented Jan 13, 2021

@smeijer the trick with useLocation().params is that the location object returned from that hook is global to <LocationProvider>, so it's possible to have multiple routes (and their path patterns) simultaneously be "current":

const App = () => (
  <LocationProvider>
    <ErrorBoundary>
      <div id="app">
        <header>
          <Router>
            <Profile path="/profiles/:profile" />
          </Router>
        </header>
        <Router>
          <Profile path="/profiles/:id" />
        </Router>
      </div>
    </ErrorBoundary>
  </LocationProvider>
);

Another option that was floated was to allow passing a path pattern to useLocation() itself:

const { id } = useLocation('/profiles/:id').params;

It's more direct, but I wonder if it'd be workable?

@smeijer
Copy link

smeijer commented Jan 14, 2021

@developit, I understand. I've made a small sandbox to show that react-router doesn't care about that. Don't get me wrong, I'm not saying that this project must follow their example, I respect that this is a different project, and can have its own approach. Just sharing to show that my first expectation would be due to my experience with them.

Your example const { id } = useLocation('/profiles/:id').params; is definitely a nice start. It does have an impact on the developer experience tho, as we now need to connect components to url (patterns) ourselves.

As an alternative, I was thinking to simply merge all the paramters, and return the merge result instead. Your example from above would lead to const { id, profile } = useLocation().params in both Profile components, because one specified id, and the other profile. But that might lead to unexpected side effects.

Ideally, the params would be scoped to Router instead of LocationProvider.

It's more direct, but I wonder if it'd be workable?

Definitely workable.

@developit
Copy link
Member

Merging all parameters seems like a pretty reasonable default. I'll have to noodle on how to implement it though...

Another option I thought of would be to have <Router> re-define the useLocation context object. That would let each Router pass only its current { url, path, query, params } object through the tree, which would support the react-router approach your sandbox uses. Maybe that's the easiest way to go.

@smeijer
Copy link

smeijer commented Jan 14, 2021

The "other option" looks to me like the preferred way to go. (to have <Router> re-define the useLocation context). If that's the easiest one too, we have a win-win situation. (It's also what I meant with:

Ideally, the params would be scoped to Router instead of LocationProvider.)

Merging looked like the easy solution to me in case the above wouldn't be possible, but the side effects might not be worth it. Especially if the better option can be done.

I mean, merging creates the situation where one might depend on the absence of params, while another unrelated route is setting them. When developers work with generic names like /:view/:tab/:page/:id it's a matter of time before that happens.

Base automatically changed from master to main January 22, 2021 18:07
@JoviDeCroock
Copy link
Member Author

Superseded by #354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using parameters with preact-iso/router
4 participants