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

feat: add new 'useCreateHref' and 'useResolvePath' hooks #9161

Closed
wants to merge 1 commit into from

Conversation

jonkoops
Copy link
Contributor

This PR implements two new hooks that make it possible to resolve paths and create hrefs without specifying a to immediately. The reasoning for this is that it allows these methods to be passed to non-React functions where it would not be possible to use hooks or allow iterating over paths without wrapping them in an additional component.

For example:

const routes = [
  { pathname: '/a/b' },
  { pathname: '/c/d' },
];

function App() {
  const createHref = useCreateHref();

  return (
    <ul>
      {routes.map(to => <li>{createHref(to)}</li>)}
    </ul>
  );
}

vs.

const routes = [
  { pathname: '/a/b' },
  { pathname: '/c/d' },
];

function App() {
  return (
    <ul>
      {routes.map(to => <ListItem to={to} />)}
    </ul>
  );
}

function ListItem(to) {
  const href = useHref(to);
  return (
    <li>{href}</li>
  );
}

Although the latter might in some cases be preferable it's not always possible or wanted to create a wrapper component. For example in our application we have a utility function that is used to build props for another component:

https://github.com/keycloak/keycloak-ui/blob/f76259088ee057be54f00e34dbac0ad88af96060/src/components/routable-tabs/RoutableTabs.tsx#L74-L77

Which is then used by spreading the resulting props over said component:

https://github.com/keycloak/keycloak-ui/blob/a6fd2cabfa4f802590b7f75d509326447606102f/src/realm-settings/user-profile/UserProfileTab.tsx#L40-L43

For our application this is required as only specific components are allowed as children where these props are spread, so wrapping things in another component and calling useHref() is not possible as these props would not match.

Closes #8292, closes #8172

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2022

🦋 Changeset detected

Latest commit: 373bc44

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
react-router Minor
react-router-dom Minor
react-router-dom-v5-compat Minor
react-router-native Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jonkoops
Copy link
Contributor Author

jonkoops commented Dec 6, 2022

@brophdawg11 is there still any interest in getting this in? This is the last thing we need to migrate our application over to React Router v6 and I'd like to know if I need to start thinking about workarounds rather than upstreaming this.

@jonkoops jonkoops force-pushed the use-create-href branch 3 times, most recently from 6882944 to c6d3f9b Compare December 6, 2022 17:51
@jonkoops
Copy link
Contributor Author

jonkoops commented Jan 3, 2023

Any chance this will be reviewed or at least get some feedback? Otherwise I might just close this PR instead.

@brophdawg11
Copy link
Contributor

Hey @jonkoops - sorry it was quite a busy end of 2022 for us. We've recently moved to a more open (and formalized) development process - so with that in mind it's probably best to write this up as a formal proposal to be reviewed. I also suspect that any change like this would be impacted by this proposal as we'd be moving a lot of the relative routing logic out of the react-router layer and into @remix-run/router.

Do you want to close this PR for the time being and we can re-open if the proposal is accepted?

@jonkoops
Copy link
Contributor Author

jonkoops commented Jan 6, 2023

No problem, just wanted to know what to expect. I'll follow the proposal an close this PR for now.

@jonkoops jonkoops closed this Jan 6, 2023
@jonkoops jonkoops deleted the use-create-href branch January 6, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants