Update README.md with example for react-router v5

[sebelga]

This PR adds the documentation on how we can forward the router with v5.

I found the solution while testing with react-router 5.0.0 installed and had to make my tests pass.
You can see here the change I had to make: https://github.com/elastic/kibana/pull/36871/files#diff-45ba9f8ef2d45b26681f545635cb504dR24

[chandlerprall]

@sebelga do you have an example of this code in use? I feel I'm missing a piece of what it does

[sebelga]

@chandlerprall you can view it here:
https://github.com/elastic/kibana/blob/0ccc9b49d70b22fcc7ed22a1cf1f2f2b718d864c/x-pack/test_utils/router_helpers.tsx

It is not yet merged in master though.

[cjcenizal]

@chandlerprall I left some comments to try to explain what I believe is happening in the proposed code. @sebelga Please correct me if I'm wrong.

@pickypg Since you originally documented this problem (#1778) would you mind testing out the proposed solution and verifying it works? No worries if you're too busy. 😄

[cjcenizal]

I think I understand what this code is doing. Here, registerRouter is the function from the routing.js example in the "react-router 4.x" section above.

[sebelga]

Exactly 😊

[cjcenizal]

registerRouter is passed as the onRouter parameter here. Perhaps the name is confusing? Maybe registerRouter or dispatchReactRouter would be clearer?

onRouter is called on mount, which is when we register the router, except...

...on line 264, we're constructing an object that is intended to substitute for a reference to the router object in the React Router API. However, it looks like this is the incorrect shape. From the docs, it looks like routing.js expects the router shape to be:

{
  location,
  createHref,
  push,
}

[sebelga]

Mmmm ok, here I follow the basic react pattern

const onClick = (event) => { ... };

return (
  <button onClick={onClick} />
)

As we are providing a handler for when the router is ready, I went for onRouter 😊

Both names don't have to match as routing.js could give any name to its router register handler (setRouter(), registerRouter(), onRouter(), whenRouterIsReady(). Each app will have it differently. But if you think that for the documentation it makes it clearer we can make those names match.

I was thinking that the catchRouter HOC in the example could be provided by EUI.

For the deconstruct, the full router are those 3 props, what routing.js is expecting is just the history it seems.

EDIT: You were right, the shape of the router was not right. I just pushed a commit to fix it.

[cjcenizal]

I just realized this follows the adapter design pattern. How about using that term to make its purpose clearer?

export const routerAdapter = receiveAdaptedRouter => WrappedComponent =>
  withRouter(
    class extends Component {
      componentDidMount() {
        const { match, location, history } = this.props;
        const router = { route: { match, location }, history };
        receiveAdaptedRouter(adaptedRouter);
      }

      render() {
        return <WrappedComponent {...this.props} />;
      }
    }
  );

[pickypg]

Unfortunately I won't have time to properly review this by injecting it into my own code this week. I had switched to hookrouter after running into this issue and kind of being dissatisfied with React Router (after some convincing :)).

[cjcenizal]

@pickypg No worries, thanks!

[chandlerprall]

Sorry for the delay @sebelga , and thanks to you and @cjcenizal for the clarifying thoughts! These changes LGTM

[sebelga]

No worries @chandlerprall, thanks for the review! Do you think EUI would be interested in providing the HOC for the teams?

[cjcenizal]

Do you think EUI would be interested in providing the HOC for the teams?

This gets tricky because we'd want tests using the actual v5 react-router to make sure the interface that gets adapted hasn't changed, but then that creates a dependency upon v5 react-router in EUI, which is outside of EUI's scope. So I would say this should be implemented by end consumers, not provided by EUI.

[sebelga]

@cjcenizal I am not sure I follow your thought process here. This would be a HOC to be used for teams wanting to use react-router 5. And it seems that withRouter is available since v4 so should be compatible.

[cjcenizal]

@sebelga If EUI publishes a tool then I assume it will need to be something that's tested. Since the HOC adapts the shape of the v5 react-router to another shape, the interface it publishes is dependent upon the v5 react-router interface. For example, if the v5 react-router shape changes from { route: { match, location }, history } to { match, location, history } or some other breaking change then the adapter will be "broken" and we would want the test to catch that.

[chandlerprall]

I agree with @cjcenizal and will add some additional thoughts -

The react-router+eui integration is a hack. We've documented the hack and want to provide a good developer experience when working with react-router, but fundamentally react-router doesn't integrate well with any anchor element provided by a component library. For example, @atlassian/atlaskit's button/link and breadcrumb react-router examples. where the Link component is passed through as an override (open Source tab): button, breadcrumb. This is an ugly thing a developer has to do every time they want a react-router link.

We could allow the same pattern in EUI and allow custom components to be passed, but would lose a lot of guarantees (especially accessibility) we are currently able to provide. It's also (IMO) not a good developer experience. So, we opted for a different hack, where the application is responsible for extracting the router's internals and interacting with them, but at least the code is largely a one-time addition, and it's takes a less ugly hack every time you use EUI's link or button.

But we chose to document the hack instead of provide it out-of-the-box as a component because every application is different. We don't know the react-router version someone will use, and supporting even the most recent + one previous is difficult on the EUI side (per @cjcenizal's comments). We don't know their application structure, and how/where it's okay to store or expose the router, so at most we are adding the catchRouter HOC (which, to be fair, is all you proposed). But that's ~12 lines of code that are easy to copy&paste - and a developer is likely to modify it to better fit their application.

To provide the HOC, we would also need to include react-router as a peerDependency, as it relies on withRouter. I already find installing EUI into a project annoying as it requires manually installing moment and @elastic/datemath even if I'm not going to actually use them. If I were a dev wanting to try out EUI and I saw that it required installing react-router, I would seriously re-consider.

[sebelga]

@cjcenizal Yes, the HOC provided by EUI should have its tests, still not seeing where is the problem in providing the HOC for all the teams to use 😊

[cjcenizal]

@sebelga Ahh sorry you're right, I didn't go into enough detail. The problem is that react-router may change its interface between minors, causing our HOC to break. And for that matter, we have no guarantee that the consumer would be using a specific minor. So IMO, making v5 of react-router part of EUI's interface introduces a lot of complexity (Chandler mentioned other complex aspects to consider, above), without a whole lot of benefit.

[sebelga]

Thanks for the explanation @chandlerprall , it makes sense. As talked with @cjcenizal I updated the name of the HOC to extractRouter which is clearer. I will merge the PR.