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

[v6] Add <HistoryRouter> for standalone history objects #7586

Merged
merged 4 commits into from Dec 2, 2021

Conversation

salvoravida
Copy link
Contributor

#7585

Hi guys, great work with v6 release!

Just a little note:

It would be nice to add support for a already created history object on <BrowserRouter
and skip creation if it is passed as prop.

We need it to support redux-first-history integration.

@salvoravida salvoravida changed the title add history props to browserrouter add history prop to <BrowserRouter Aug 26, 2020
@salvoravida salvoravida changed the title add history prop to <BrowserRouter [v6] Add history prop to BrowserRouter Aug 26, 2020
@timdorr
Copy link
Member

timdorr commented Aug 26, 2020

I think rather than overriding the internal history of BrowserRouter, it would make more sense to create a HistoryRouter in the react-router package that takes a history prop. That would be more generic and would potentially allow us to abstract this repeated pattern of creating a history and listening to it.

@salvoravida
Copy link
Contributor Author

I think rather than overriding the internal history of BrowserRouter, it would make more sense to create a HistoryRouter in the react-router package that takes a history prop. That would be more generic and would potentially allow us to abstract this repeated pattern of creating a history and listening to it.

Yes, i agree!

I have modified the PR with HistoryRouter. Let's have a look!
Regards
Salvo

@salvoravida
Copy link
Contributor Author

Sorry for the request, i havent seen that you have already approved.

@salvoravida
Copy link
Contributor Author

Hi Tim, can we see this pr merged on nest beta?

@timdorr
Copy link
Member

timdorr commented Sep 7, 2020

Michael or Ryan would need to approve this. They have a vision for v6, so they need to provide input as to whether this fits that or not.

@ejose19
Copy link

ejose19 commented Sep 29, 2020

Would be great if this landed on v6, as useNavigate/useBlocker falls short in some cases where the history object can solve it directly (like navigating in some places where the hook is not available, or adding more history listeners to have more control in certain actions)

@eturino
Copy link

eturino commented Oct 20, 2020

We had an issue with Authentication. Our Auth provider needed to use navigate, that needed to be inside of the Router, but since every child of Router gets re-rendered, the Auth run again.

The solution was to use this HistoryRouter, have the Auth Provider use history instead of useNavigate, and then give the HistoryRouter the same instance of history.

I think this is needed.

@michaelx
Copy link

michaelx commented Nov 18, 2020

Same case as @eturino. Would love to see it being picked up. Really appreciate the PR and v6.

@ejose19
Copy link

ejose19 commented Jan 9, 2021

@salvoravida While using this without being merged, I had some use cases were I needed to alter newState or trigger actions directly at the listener (since secondary listeners were run later), so I expanded your component to also receive a reducer and historyListener as prop:

/**
 * A <Router> for use in web browsers with custom history. Provides the cleanest URLs.
 */
export function HistoryRouter({
  children,
  history,
  reducer = (_: Update, action: Update) => action,
  historyListener
}: HistoryRouterProps) {
  let [state, dispatch] = React.useReducer(reducer, {
    action: history.action,
    location: history.location
  });

  React.useLayoutEffect(
    () =>
      history.listen(
        historyListener !== undefined
          ? action => historyListener(action, dispatch)
          : dispatch
      ),
    [history, historyListener]
  );

  return (
    <Router
      children={children}
      action={state.action}
      location={state.location}
      navigator={history}
    />
  );
}

export interface HistoryRouterProps {
  children?: React.ReactNode;
  history: BrowserHistory;
  reducer?: (state: Update, action: Update) => Update;
  historyListener?: (action: Update, dispatch: React.Dispatch<Update>) => void;
}

if (__DEV__) {
  HistoryRouter.displayName = 'HistoryRouter';
  HistoryRouter.propTypes = {
    children: PropTypes.node,
    history: PropTypes.object,
    reducer: PropTypes.func,
    historyListener: PropTypes.func
  };
}

Feel free to merge it in your PR if you find it useful!

@salvoravida
Copy link
Contributor Author

hi @ejose19 we need this pr for support redux-first-history. i think we do not need extra listener.

@timdorr could we see this pr merged next beta?

@jamime
Copy link

jamime commented Jan 11, 2021

I have the same request, but our use case is different.

We're using single-spa to build micro-frontends. We want to use the same history instance in multiple micro-frontends. This will allow us to use history.block in one micro-frontend to prevent navigation triggered by a different micro-frontend.

It would be great to see HistoryRouter added.

@salvoravida
Copy link
Contributor Author

Hi Tim, i have updated the pr, without proptypes as others Router.

@n19htz
Copy link

n19htz commented May 2, 2021

if you use stack with redux-saga you could run into this screnario:
for example you saving Order via redux-sagas (saveOrderRequestAction - dispatched from a components via redux dispatch, saveOrderSuccessAction, saveOrderFailureAction). And when you successfuly saved Order(saveOrderSuccessAction triggered by saga via put) you need to navigate to newly created order page i e /orders/newOrderId, where newOrderId obtained only after Order is actually saved. With react-router-6 you got the only option to trigger page navigation is via navigate(NavigateFunction) function passed to initial saveOrderRequestAction wich is violation of redux state normalization, since functions can't be serialized, you could also use window.history.go(delta), but thats not an option here since you need to go to specific page url and not just go back/forward on n amount of pages.
So with this in mind, allowing to pass custom history to a BrowserRouter would than solve such kind of problem, since you could just simply export history passed to BrowserRouter and use it globally in sagas by simply importing it there. Am I wrong and this kind of scenario could be solved without flux violations in stack with redux-saga?

@salvoravida
Copy link
Contributor Author

salvoravida commented May 2, 2021

@n19htz

https://github.com/salvoravida/redux-first-history

you can use put(push) from redux-first-history (that works already for rr5.) we also need this PR to support rr6

@salvoravida
Copy link
Contributor Author

@timdorr could we see this pr merged next beta?

@kraegpoeth
Copy link

Whats the status of this? :)

@mjackson
Copy link
Member

Thanks for the PR, but you can already do this in v6 with a regular <Router> element.

let history = createHistory();

<Router location={history.location} navigator={history}>
  ...
</Router>

A <Router> is rendered by each high-level router (<BrowserRouter>, <HashRouter>, etc) so you would be using the same interface as all the other routers.

@salvoravida
Copy link
Contributor Author

salvoravida commented Aug 16, 2021

Whats the status of this? :)

I have the same request, but our use case is different.

We're using single-spa to build micro-frontends. We want to use the same history instance in multiple micro-frontends. This will allow us to use history.block in one micro-frontend to prevent navigation triggered by a different micro-frontend.

It would be great to see HistoryRouter added.

Meanwhile this PR is merged, I have added it on redux-first-history@5.0.0-beta.2

import { HistoryRouter as Router } from "redux-first-history/rr6";
import { Route, Routes, Navigate } from "react-router-dom";
//...
import { store, history } from "./store";
//...

     <Router history={history}>
                 <Routes>
                   <Route path="/dashboard" element={<Dashboard />} />
                   <Route path="/" element={<Home />} />
                   <Route path="*" element={<Navigate to="/" />} />
                 </Routes>
     </Router>

demo rr6 here: https://codesandbox.io/s/redux-first-history-demo-rr6-uccuw

@mjackson
Copy link
Member

mjackson commented Aug 16, 2021 via email

@ejose19
Copy link

ejose19 commented Aug 16, 2021

I'm also puzzled why this won't be merged, it's a requested feature and makes users write less code for common use cases.

@salvoravida
Copy link
Contributor Author

@h3rmanj @ejose19
that's why i decided to put it on my lib

import { HistoryRouter } from "redux-first-history/rr6";

or save to HistoryRouter.js on your project:

import React from 'react';
import { Router } from 'react-router';

export function HistoryRouter({ children, history }) {
  const [state, setState] = React.useState({
     action: history.action,
     location: history.location,
  });
  React.useLayoutEffect(() => history.listen(setState), [history]);
  return React.createElement(Router, Object.assign({ children, navigator: history }, state));
}

@mjackson
Copy link
Member

I think I may have been too focused on the Redux use case when I commented earlier. <HistoryRouter> may be useful in other scenarios too, so apologies for closing. I'm going to rename the issue and reopen.

@mjackson mjackson reopened this Aug 16, 2021
@mjackson mjackson changed the title [v6] Add history prop to BrowserRouter [v6] Add <HistoryRouter> for standalone history objects Aug 16, 2021
@mjackson mjackson removed the fresh label Sep 9, 2021
@Nantris
Copy link

Nantris commented Sep 21, 2021

Any movement on the HistoryRouter concept? I think we need it, since we have a custom history object. I say "I think" because of @salvoravida. I'm surprisingly happy to have hit this issue since it's brought me upon the redux-first-history library, but I've yet to give it a try.

@salvoravida I assume it should still work with the latest react-router beta?

@salvoravida
Copy link
Contributor Author

Any movement on the HistoryRouter concept? I think we need it, since we have a custom history object. I say "I think" because of @salvoravida. I'm surprisingly happy to have hit this issue since it's brought me upon the redux-first-history library, but I've yet to give it a try.

@salvoravida I assume it should still work with the latest react-router beta?

yes you can test https://github.com/salvoravida/redux-first-history/releases/tag/v5.0.0-beta.2

@ejose19
Copy link

ejose19 commented Nov 13, 2021

This PR needs an update for latest release (6.0.2) as now it's issuing this error:

Type '{ children: ReactNode; action: any; location: any; navigator: any; }' is not assignable to type 'IntrinsicAttributes & RouterProps'.
  Property 'action' does not exist on type 'IntrinsicAttributes & RouterProps'.ts(2322)

@salvoravida salvoravida force-pushed the dev-v6-add-history-prop branch 2 times, most recently from 4298778 to cc31f39 Compare November 13, 2021 13:37
@salvoravida
Copy link
Contributor Author

This PR needs an update for latest release (6.0.2) as now it's issuing this error:

Type '{ children: ReactNode; action: any; location: any; navigator: any; }' is not assignable to type 'IntrinsicAttributes & RouterProps'.
  Property 'action' does not exist on type 'IntrinsicAttributes & RouterProps'.ts(2322)

done!

@alecglen
Copy link

alecglen commented Dec 1, 2021

What is the timeline for getting this in a release?

@timdorr
Copy link
Member

timdorr commented Dec 2, 2021

Based on Michael's comments earlier, let's get this merged in for the next release.

@timdorr timdorr merged commit 8a04a65 into remix-run:dev Dec 2, 2021
timdorr added a commit that referenced this pull request Dec 3, 2021
Co-authored-by: Tim Dorr <git@timdorr.com>
@mjackson
Copy link
Member

mjackson commented Dec 11, 2021

I was dragging my feet on doing anything about this because history is a regular dep of the router in v6, not a peer dep. So anyone who uses the <HistoryRouter> we shipped in 6.1 needs to use the same version of history we are using in React Router or they’ll get two versions of the history library in their bundles.

Sorry I wasn’t more communicative about this. I sympathize with the OP and everyone who needs this feature, but we have definitely created another potential bundling issue by merging this work.

@alecglen
Copy link

Good point, and thanks for working to give us an option in spite of that! I agree it feels a little dirty since a lot of v6 tries to abstract away history. I'm definitely open to alternatives, so long as it solves the problem summarized in #8342.

For what it's worth, my project and hopefully the majority of React projects out there only included history to provide it for react-router, so I was able to just remove it after the upgrade making that a non-issue.

@salvoravida
Copy link
Contributor Author

salvoravida commented Dec 14, 2021

I was dragging my feet on doing anything about this because history is a regular dep of the router in v6, not a peer dep. So anyone who uses the <HistoryRouter> we shipped in 6.1 needs to use the same version of history we are using in React Router, or they’ll get two versions of the history library in their bundles.

Sorry I wasn’t more communicative about this. I sympathize with the OP and everyone who needs this feature, but we have definitely created another potential bundling issue by merging this work.

I understand your concerns, but honestly, I do not see any potential bugs allowing just to use an external history object:

That said, great work with the new react-router v6! Regards!

@yann-combarnous
Copy link

I was dragging my feet on doing anything about this because history is a regular dep of the router in v6, not a peer dep. So anyone who uses the <HistoryRouter> we shipped in 6.1 needs to use the same version of history we are using in React Router, or they’ll get two versions of the history library in their bundles.
Sorry I wasn’t more communicative about this. I sympathize with the OP and everyone who needs this feature, but we have definitely created another potential bundling issue by merging this work.

I understand your concerns, but honestly, I do not see any potential bugs allowing just to use an external history object:

That said, great work with the new react-router v6! Regards!

Just to add to this, for Micro-frontends, it is also useful to pass the history object to other apps on same page. See https://blog.bitsrc.io/how-to-develop-microfrontends-using-react-step-by-step-guide-47ebb479cacd for React-router v5 implementation.

For v6, we would need to clone the BrowserRouter Object to be able to pass a history object.

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.

None yet