Fix React-Router warnings

[vanadium23]

When you open console on admin edit pages, there are react router warnings. Better fix them, before it's too late (:

Warning: [react-router] It appears you have provided a deprecated history object to `<Router/>`, please use a history provided by React Router with `import { browserHistory } from 'react-router'` or `import { hashHistory } from 'react-router'`. If you are using a custom history please create it with `useRouterHistory`, see http://tiny.cc/router-usinghistory for details.
warning
Warning: [react-router] `props.history` and `context.history` are deprecated. Please use `context.router`. http://tiny.cc/router-contextchanges
warning
Warning: [react-router] adding multiple leave hooks for the same route is deprecated; manage multiple confirmations in your own code instead

[runfalk]

I did some digging into this. It seems like there have been two major releases since the version Lektor uses. It probably makes sense to migrate to the latest version (4 from 2.8.1) to ensure future compatibility.

[nixjdm]

@runfalk Agreed. I'll make a separate ticket for updating the node requirements though, since at the moment, the warnings are gone.

[runfalk]

The warnings are not gone though. This is the current master (after my patch was merged):

Unless I'm missing something.

[nixjdm]

@runfalk You're right, my mistake. I thought I checked this but looking again you're totally right. Thanks for the catch.

[runfalk]

I've done some testing and tried to wrap my head around the current code base. React router made some pretty breaking changes in v4 (we're on v2). I think the only reasonable way is forwards, but there are some design problems. The main reason to not stay on v2 is that it has deprecated use of React.PropTypes as well as being unmaintained. This means we can't upgrade to React 16 when it's released.

The current structure is that the App component is mapped to the /admin route, and it has children for each of the child pages such as :path/edit, :path/preview, :path/add-child, etc. Its cool and all that things are decoupled, but the sidebar, and breadcrumbs are very coupled which kind of defeats the purpose of injecting the view components as children of App. One cool feature could've been that multiple App instances could co-exist on the same page but they all share a global event bus for the dialogs that pop up, so that doesn't work. Every component between within the App instance can deduce it's own URL as well as construct URLs depending on whether it has received special properties from its parents. The whole thing is kind of unweildy.

I think the logical solution is to make App the binding between all things. The App will no longer take children, and instead include all components necessary to function like it currently does. It'd lose some flexibility, that I don't think is needed anyway.

This turned out longer than I intended. Am I missing something, or should I get to coding this?

[nixjdm]

@runfalk I don't think you're missing anything about this code, but honestly it's all a bit hard for me to grok too. I've mentioned before, React is not my forte. That said I think I follow what you're saying and it makes sense.

I have a some other thoughts though.

I'd love to release a new version of Lektor without any known new errors. Warnings don't really present to users well either. How big of a task do you estimate it is for you? It doesn't sound easy. If you think you can do it in a reasonable timeframe, that sounds great. I would really appreciate it. Not being a React expert, I can offer only limited help. If you don't think you can do that very quickly, I'm thinking you should probably hold off altogether because of the next point.

If you haven't seen, we're think thinking about replacing the React code at some point, for various reasons. I don't think anyone is really happy with it. I want you to know this is being discussed before dumping in a ton of effort. Replacing the admin is not a simple task though. If we do that, it probably won't happen overnight. I don't want to do that before the next release. Your understanding of how the admin currently works would be valuable for this too, so if you're at all interested, please stick around regardless of how these warnings are dealt with. :)

[runfalk]

So, I chose to help some with the React stuff as a way to dive into React. I've done front-end work previously but never worked on React other than toy examples. This would probably take me two full days, with the amount of reading and thinking I need to do.

If you plan on moving forward with dropping React, in the future, I really think there should be an issue opened for discussion, and a tag for issues that it might invalidate. I presume the decision to move away would be based solely on the unfortunate licensing of React.

I'll stick around 😼

[nixjdm]

I have no problem at all waiting two days, or even a week if need be. That's fast enough. :) Thanks!

[runfalk]

That'd be two full working days, which will be a lot longer since it's my spare time. I'll put it on hold until the React issue is resolved.