Refactor frontend routing to react-router

[chillu]

The current page.js based routing was a "least common denominator approach", where routing functionality was stripped back to simple callback handling. See #5243 for context. The rationale was that a react based router couldn't handle Entwine/PJAX based legacy sections cleanly. This has caused a lot of work in the last week, combined with a lot of learning about how routing should influence views and redux state.

Some disadvantages:

• All but the innermost components need to be aware of the routing hierarchy, with switch statements and clumsy route state management in React/Redux vs. page.js. Check the react-router introduction to get an understanding how this approach differs for standard callback-based routing, see "Without React Router".
• AssetGalleryField was designed as a standalone, "semi-Entwine" component with its own React root initialisation based on Entwine matching, and its own redux state. This doesn't fit into the bigger picture of how we want the different UIs to work together
• Since page.js doesn't know about React, it can't support users in nested views - leading to single components with too many responsibilities (the switch(this.props.view) conditional in AssetAdmin is a good example)
• Same goes for breadcrumbs - since there's no sense of which part of a route is rendered by a component, there's no support for creating breadcrumbs automatically in page.js
• Lazy route registration in page.js is possible (after calling page.start()), but there's no introspection into existing routes which can be limiting for customisation. With react-router, we can keep all routes in an SS-managed registry and pull them on demand.

We're attempting to get react-router into core as an optional router which just kicks in for new react-based sections. We might use react-router-redux (not redux-router since its fallen out of favour). It's technically not required at the moment (we can pass props.params through to children components), but seems likely to be required for any additional components from third party extensions, since those won't have easy access to props.params (note that redux-router recommends not to use this redux state directly though).

Branches:
https://github.com/open-sausages/silverstripe-framework/tree/pulls/4.0/react-router
https://github.com/open-sausages/silverstripe-asset-admin/tree/pulls/4.0/react-router

Tasks

• Switch between legacy and react routing on boot
• Rewrite lib/RouteRegister to work with react-router config instead of callbacks
• Remove PJAX loading of main sections in menu (full page reload required for router switching)
• Allow injection of components outside framework into FormBuilder
• Separate out AssetGalleryField logic from AssetAdmin container so it can be lazy loaded via FormBuilder (we removed AssetGalleryField instead, not relying on form schema for this part)
• Share redux state and reducers between framework and asset-admin modules
• Allow lazy registration of routes in react-router configuration (via getChildRoutes callbacks)
• Lazy registration of admin/assets/show/:folderId/edit/:fileId route via AssetGalleryField (no longer required since we removed that indirection)
• Render Editor through react-router as a childRoute of AssetGalleryField
• Re-match route once AssetGalleryField has lazy registered routes (might need to show Editor component)
• Use react-router :folderId and :fileId props for view initialisation
• Remove dependance of AssetAdmin init on jQuery.entwine, use FormBuilder instead
• Fix router.show() calls throughout CMS
• Use browserHistory.push() instead of router.show() in CampaignAdmin and AssetAdmin (docs)
• Update changed propTypes on all components

Things that block merge:

• Introduce react-router-redux (Ingo's working this now) to populate redux state from URL (albeit asynchronously so can only be read at certain times)
• Fix "flash of editor view" on admin/assets load
• Fix editor now showing in responsive view (probably just a HTML nesting bug)
• Sync back Redux state into route, and identify legal times to read react-router-redux state (see current implementation through refreshUrl), and rationale from Redux creator Pulled out into Minor: review handling of redux state with react-router. #5797
• If "Sync back Redux state into route" is a dead end: Ensure that all existing reducers are accessing router params and not legacy state values (state.gallery.fileId and state.gallery.folderId) Pulled out into Minor: review handling of redux state with react-router. #5797
• Fix Cannot read property 'replace' of undefined error on startup (history state doesn't have all required properties)
• Use persistent backend.createEndpointFetcher() rather than recreating on each render() call in AssetAdmin
• Write tests for legacy/react router switching
• Cross-browser testing
• Test for state management regressions
  • Open a folder, highlight files, navigate to a different folder, highlight other files, delete those => should not delete originally highlighted files
  • Open editor, close it again, ensure the URL updates. Navigate back and forward in browser.
  • Open edit for a file, then also select the checkbox on that file and delete it => the editor should close
• Fix AssetAdmin container layout regression (too much padding). May be "ensure HTML is good and give to Paul"
• Update routing docs (mention new reactRouter boolean flag in LeftAndMain->getClientConfig())

Things that don't block merge:

• Clean up legacy routing:
  • Refactor lib/Router to avoid wrapping around page.js (ideally we don't need a custom page.show()).
  • Avoid full page reload in CMS menu when navigating between sections with the same router. Already the case in react router, maybe not in the legacy issues.
• Tidy up asset breadcrumbs:
  • Fix back() navigation on breadcrumbs (use new browserHistory object) (docs)
  • Infer breadcrumbs from route stack (ideally each component can contribute one or more breadcrumbs, see example

Related

• Remove entwine/PJAX dependency from AssetAdmin boot logic (Remove entwine/PJAX dependency from AssetAdmin boot logic silverstripe-asset-admin#131)
• File edit doesn't update URL after navigating to different React section (BUG: File edit doesn't update URL after navigating to different React section silverstripe-asset-admin#193)
• IE10+ doesn't update URL on folder navigation or file display (IE10+ doesn't update URL on folder navigation or file display silverstripe-asset-admin#188)
• Refactor and into containers and presentational components (Refactor <Gallery> and <Editor> into containers and presentational components silverstripe-asset-admin#179)
• Use FormBuilder in file edit form (Use FormBuilder in file edit form silverstripe-asset-admin#171)
• Implement breadcrumbs (Implement breadcrumbs silverstripe-asset-admin#158)

Recommended Reading

• David/Ingo React research from early 2016 (originally recommended page.js): https://docs.google.com/document/d/1MXDhNPz60Lt5X4VlyB6hFUJGl3H6vIPxPAqV2lmdMnI/edit#
• Intro to react-router-redux: http://jlongster.com/A-Simple-Way-to-Route-with-Redux
• Good full app example: https://github.com/emmenko/redux-react-router-async-example
• Best practice from Redux author on state/route sync: firing actions in response to route transitions in react-router reduxjs/redux#227 (comment)
• Wordpress data persistence evolution: https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md#third-era-redux-global-state-tree-december-2015---present

[chillu]

react-router PoC: https://gist.github.com/chillu/b1cdda9b49d98a257aae1ba8cd4d174b

[chillu]

We're initialising the react-router history object with a custom base, so can't just do import { browserHistory } from react-router throughout the codebase. For react components, this is solved through the withRouter() HOC, which allows this.props.router.push() within the component. For redux actions, we can't use this process. Even though we could create lib/History as a singleton, it won't be useable until the react init runs after onDocumentLoaded through framework/admin/client/src/boot/index.js (and the history object is instanciated).

So just in case we need a history reference in actions (for changing the URL based on actions), I'd recommend passing it through redux-thunk: https://github.com/gaearon/redux-thunk#injecting-a-custom-argument

[chillu]

Regarding react-router vs. redux: Take the admin/assets/show/:folderId route. react-router passes down the params as this.props.params.folderId into the rendered component (<AssetAdmin>). In AssetAdmin.mapStateToProps we're setting folderId: ownProps.params.folderId. AssetAdmin.render then passes down down the prop through <Gallery folderId={this.props.folderId}>. So folderId never makes it into the redux store - single source of truth.

This is a different approach to the GalleryActions.setFolder() action, which used to set the active folder directly in redux (see https://github.com/silverstripe/silverstripe-asset-admin/blob/73fa20c361c275a81ff57e4b5b02c338086f3b83/client/src/containers/AssetAdmin/AssetAdmin.js#L116).

While the new approach looks cleaner on the surface, it has a serious limitation: Since GalleryActions.setFolder() is never called, the actions can't cause other state modifications, e.g. unsetting HIGHLIGHTED_FILES (https://github.com/silverstripe/silverstripe-asset-admin/blob/73fa20c361c275a81ff57e4b5b02c338086f3b83/client/src/state/gallery/GalleryActions.js#L8). While this could arguably be moved into a separate reducer which responds to the GALLERY.SET_FOLDER action, there's still no entry point for the action to be fired in the first place.

Another use case is closing the <Editor>, which used to call GalleryActions.setFile(null). Under the new system, it would need to call router.push('admin/assets/show/<currentFolderId>'), which feels messy. Same goes for GalleryActions.createFolder(), which should redirect to the new folder after creating it. Calling routes from action creators is messy as well, since action creators shouldn't know about these view concerns.

While react-router-redux stores params in the state.routing, it's not supposed to be used in mapStateToProps (details). reduxjs/redux#637 and reduxjs/redux#227 (comment) discuss a potential solution for this, by having a reducer listen to a custom history event. This event only has the location payload, not the resolved params for the route. The only workaround which I've seen talked about is using react-router's onEnter callback to fire an action: reduxjs/redux#637 (comment) - seems like a lot of boilerplate.

So I'm back at square one with this ... https://github.com/acdlite/redux-router would solve the issue, but it's unclear if choosing this library is sustainable (discussion).

And once we solved the issue of getting route state into the store, we then also have to solve how we sync any state changes back to the url (current approach: https://github.com/silverstripe/silverstripe-asset-admin/blob/73fa20c361c275a81ff57e4b5b02c338086f3b83/client/src/containers/AssetAdmin/AssetAdmin.js#L76)

[chillu]

@unclecheese @assertchris Any insights on the react-router vs. redux conundrum above? :)

[tractorcow]

So folderId never makes it into the redux store - single source of truth.

Yes it is in redux store. It's in the routing store, managed via react-routing-redux.

This property is made available to components via react-redux-router syncing with react-router history object, which parses and populates the component properties.

This behaviour acts in PLACE of connect() mapping state to props directly, but it does the same thing, albeit with react-routing (and thus the browser bar) as an intermediary. However, this doesn't mean that the state isn't the source of truth. Time travel actions are synced from the store automatically by react-route-redux, so us trying to layer another state on top of that is redundant.

[tractorcow]

PR at #5796 and silverstripe/silverstripe-asset-admin#204

[chillu]

I think it comes down to how much knowledge we want third party components to have about our application structure.

Contrived example: A third party CMS module adds a better way to inline search across all files, but showing files in the current folder first. Currently this component can't just look in the Redux state for the "current folder", because its passed down from react-router directly into the component hierarchy as a prop. This is different from other UI state like "currently selected files", which is in the Redux state. Given that such a React component needs to be part of the existing component hierarchy (e.g. "injected" into AssetAdmin.render()), the parent component can decide to pass in the "current folder" context explicitly - not as clean, but acceptable.

We're also spreading knowledge of the application routing: If that new inline search wanted to close any active sidebars before displaying results, it would have to know that's done by changing the route (router.push(/${base}/show/${folderId})), rather than just calling actions.setFile(null) in a routing agnostic way. Closing the active sidebar should be a separate concern this new inline search doesn't need to worry about, and could be one of many ways the UI reacts to the changed state of actions.setFile(null).

This line here is the point of contention: You close the file sidebar by routing to the folder view, rather than setFile(null). https://github.com/open-sausages/silverstripe-asset-admin/blob/385bbe8092ea0a5761084289c711dd0900982d57/client/src/containers/AssetAdmin/AssetAdmin.js#L135

And the "selected files" Redux state is just kept in sync because its manually triggered by the component observing prop changes. This makes state transitions harder to debug than following side effects caused by actions (actions.setFolder() internally calls actions.deselectFiles). Removing this view component (but retaining the concept of a "current folder") will cause state inconsistencies unless the dev remembers to port over the refreshFolderIfNeeded() logic. https://github.com/open-sausages/silverstripe-asset-admin/blob/385bbe8092ea0a5761084289c711dd0900982d57/client/src/containers/Gallery/Gallery.js#L195

Sure, there's one source of truth for state in the current implementation, but I'm worried that it'll create an unmaintainable system if you think about the amount of components relying on the concept of a "current record" throughout different CMS interfaces.

Given that even the authors of Redux are struggling with doing this cleanly (reduxjs/redux#227 (comment)), I'll let it go for now - let's see if it becomes a problem in practice before overengineering :)

Related: RFC-9 UI Extensibility