argelius / react-onsenui-redux-weather

Onsen UI sample app using Redux and React
https://onsen.io/
MIT License
121 stars 78 forks source link

Navigator route stack is not connected to Redux state #5

Open sbj42 opened 7 years ago

sbj42 commented 7 years ago

It looks like in this app the navigator route stack and the redux state are independently controlled, which seems not to match the redux paradigm as far as I understand it.

For example, running the demo using the Redux DevTools plugin, the following steps produce an error:

  1. Start the demo clean
  2. Click 'Tokyo'
  3. In Redux DevTools, step backwards in the state history (undoing the SELECT_LOCATION action)

That causes a TypeError at WeatherPage.js:95, where country is now undefined but it's attempting to render the WeatherPage anyway because nothing has changed the Navigator's route stack. Since we've undone SELECT_LOCATION, the app should render as though no location was selected, meaning that we should return to the LocationList and WeatherPage should be disconnected. It seems like "what page we're on" should be saved in the app's state, and somehow changes to that part of the state should cause the routes to change.

Another example:

  1. Start the demo clean
  2. Click 'Tokyo'
  3. Click the back icon

In Redux DevTools, note that the navigation change did not trigger an action. The app's state does not reflect the currently-selected page, so if the app were preloaded with this state, we wouldn't be looking at the LocationList, we'd still be looking at Tokyo.

We're supposed to keep all of the app's state in the store. This is something I'm struggling with in my own app, and I don't know yet whether this can be solved with Onsen UI's Navigator, at least as available in the React API. There doesn't appear to be a way to change the route externally, though I might be missing something. I don't consider this a bug in the Weather App as a user would see it, but it's either a paradigmatic problem with the code (if it is possible to fix), or a problem with OnsenUI's Navigator (if not).

sbj42 commented 7 years ago

I'm not sure, but this may be addressed by this react-onsenui pull request.