Update history to >=5.0 and to allow transaction retry

[flash1293]

In 5.0, the history package introduced a way to retry transactions in a block hook, allowing to do asynchronous blocks. As Kibana is currently using version 4.9, this isn't possible.

history should be upgraded to 5.x and the updated transaction object with the retry method should be passed through to consumers of .scoped history

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[legrego]

According to this comment, version 5.0 fixes a long outstanding bug with routing, which impacts a few management screens in Kibana, as well as Enterprise Search: #82440.

In other words, +1 to upgrading the history package 🙂

[pgayvallet]

react-router is tightly coupled to history. I quickly tried to just bump history to 5.3.0, however without surprises, it causes type errors because of the incompatibilities of the newest version of history and the older version of history that react-router uses.

We're currently using react-router: ^5.2.0. but this version is based on history: ^4.9.0. We need to bump react-router to a later version that requires history: ^5.2.0, like the latest: "react-router": "^5.3.3"

Also, and this is the real issue here, the last version of the typings for react-router is 5.1.18 ( "@types/react-router": "^5.1.18",), which is still using history 4.x, so outdated types. We need to find a way to accept up-to-date types for react-router, else the upgrade will not be possible (react-router does not ship its types directly from the package and require a typedef)

[pgayvallet]

So, I tried to bump history to 5.0.1, as it's the last version that doesn't introduce the generic removals from History and Location, however some types are still incompatible with the last version of react-router's type ("@types/react-router": "^5.1.18").

For example, MemoryRouterProps.initialEntries is still defined as H.LocationDescriptor[], which doesn't exist anymore in any history:^5.0.0 version.

Also, react-router 5.x is based on history v4.x. To avoid breaking changes between the two libraries, we would need to update react-router to v6+ at the same time...

[pgayvallet]

Yet another thing: react-router-config, that we're using in some of our test packages, was never updated for react-router v6+, so we will likely need to check if the v5 version of this package can work with react-router v6, and if not, get rid of our usages of it.

[pgayvallet]

So, after a few hours of investigation, the only possible approach here is to bump both history and react-router (at the same time) to their highest version:

• history to 5.3.0+
• react-router to 6.3.0+

In addition, react-router-config is no longer maintained nor compatible with react-router 6, as the features are now shipped directly into react-router core package, so we will need to get rid of it and adapt usages.

Also, the version 6 of react-router drops usage of all HOC components for their hook counterpart (e.g withRouter is removed), so we would need to adapt all our code accordingly.

There are also major changes in the latest version of react-router, such as the replacement of Switch to Routes, automatic usage of relative urls for nested routes, change of format for exact/non-exact routes, suppression of useHistory or useRouter in favor of higher-level hooks (which will be a pain for a lot of our test utils such as the kbn-test-jest-helpers package) and other fancy changes we'd need to perform.

Overall, this is a major library bump impacting all our routing code in every app we'd need to do here. We're talking weeks, maybe even months, of work.

There is a compatibility mode to supposedly help with such migration. May be worth digging a bit to see if that would potentially help us, but from my experience with compat mode from other libraries (hello angular), it usually just doesn't work with complex application using a lib's API to the extreme (And still require to adapt all imports of react-router in the codebase to use the compat package instead of the default one). EDIT: yea, will not work, the main package bump is performed at the end of the migration, so history would not be updated until we actually perform the full upgrade anyway.

I opened a draft PR to take a look at it, but that's only for investigation purposes, I'm not planning on working actually on it, given the amount of work it represents.

[afharo]

Just dropping a note from the react-router docs:

If you're using React Router, you should never import anything directly from the react-router package, but you should have everything you need in either react-router-dom or react-router-native. Both of those packages re-export everything from react-router.

In other words, we may need to upgrade/install react-router-dom for our use case.

[pgayvallet]

In other words, we may need to upgrade/install react-router-dom for our use case.

Yea, sorry, by upgrading react-router to 6.x.0, I meant both react-router and react-router-dom.

[lukeelmers]

This is blocking the VisEditors team in moving away from the deprecated onAppLeave, which is targeted for removal by 8.8.

[pgayvallet]

which is targeted for removal by 8.8

Well... Which is currently targeted for removal by 8.8. If the recommended alternative (history.block with current history 4.x implementation) does not provide feature parity, we can't really remove it. We should update this deprecation notice, given the new info we have.

As much I as would love to resolve the problem by bumping those libs, as said, this is a cross-team multi week/month effort here. If supporting the new implementation of history.block / removing onAppLeave is the only outcome, I'm afraid we can't really justify the cost here, wdyt @lukeelmers?

[lukeelmers]

I'm afraid we can't really justify the cost here, wdyt @lukeelmers?

Yep I tend to agree. It is up to us whether this should be blocking the vis editors team, and deferring the removal of onAppLeave (or just deciding not to deprecate it at all) is a perfectly valid option IMO.

Unless there is any other reason to make this upgrade besides "because we are supposed to remove onAppLeave", then it feels hard to justify knowing how large of an effort this would be.

[legrego]

Unless there is any other reason to make this upgrade besides "because we are supposed to remove onAppLeave", then it feels hard to justify knowing how large of an effort this would be.

Any other reason? 😉 We still have #82440, which impacts any app which uses path-based routing (e.g. /app/myApp/user%generated%portion/), with entities that contain certain special characters, such as %. In practice, this means that any path-based route with user-controlled input is potentially susceptible.

Having said that, it sounds like the level of effort still wouldn't be justified.

[yuliacech]

Any other reason? 😉 We still have #82440, which impacts any app which uses path-based routing (e.g. /app/myApp/user%generated%portion/), with entities that contain certain special characters, such as %. In practice, this means that any path-based route with user-controlled input is potentially susceptible.

We found a workaround for the user-generated input in the path: to use it only in a query param, for example /indices/${indexName} -> /indices/index_details?indexName=${indexName} (#166882)

[TinaHeiligers]

cc @eokoneyo