neilff / redux-ui-router

ngRedux bindings for Angular UI Router
MIT License
143 stars 48 forks source link

Losing state params when refreshing the page #66

Closed Nighthawk14 closed 8 years ago

Nighthawk14 commented 8 years ago

Hi,

Testing the new ui-router 1.0.0-beta.1 compatibility #61, it occurs that when you refresh a page the state params are not propagated to the state in the store (e.g /foo/1 where /1 is /:id will results in /foo/undefined after refreshing the page and is not part of the properties fromParams and toParams of the onStart and onSuccess actions).

Trying to find out if it's an issue with ui-router or with redux-ui-router right now.

ps: I tried with and without html5location enabled ps2: I tried serving the files via webpack-dev-server or via nginx, same result.

Nighthawk14 commented 8 years ago

After a bit of investigation, it appears that the param is actually propagated to the state in the store but later than before. So let's say your state has a controller, you expect the param to be already present at instantiation of your controller. With the current redux-ui-router and ui-router this happens. With the new redux-ui-router and ui-router1.0.0-beta.1 the param is populated to the store later on and so not available in the constructor of your controller.

hally9k commented 8 years ago

If you want to submit a PR I can merge it in ASAP. Otherwise, with your awesome direction here, I should be able to patch this a bit later today. I'm just wrangling some code for cash right now otherwise I'd have a jam with you!

Nighthawk14 commented 8 years ago

Haha no worries, I checked a bit further but then I had a meeting... I want to create a plunker first with ui-router to make sure we can make it. I can actually see in the treeChanges the param populated in the onStart hook, but I don't see what is wrong with using to().params in this hook, that seems to be the right thing to do :).

Here it is: https://plnkr.co/edit/rLB7rPz7ECkB9Aicf0mL?p=preview

You can see that using $transition$.to().params in the onStart hook doesn't give you the params but using $stateParams instead is working. It doesn't make a lot of sense.. Same thing with .from().params, even navigating between the two states that contains param it's always undefined..

Edit: Found a fix. Instead of using transition.to().params which is actually sending the params object of the state property. We should use transition.params('to') which is sending the real parameters value that are passed to the state :). I'll try to PR that

Nighthawk14 commented 8 years ago

Hey @hally9k, I'm testing with the new version this morning and it appears that the issue is still there. It's definitely better because now we have the right params in the toParams property of the onStart action so that's cool. The problem is that we update the store only on STATE_CHANGE_SUCCESS but this action occurs after the instantiation of the state and it's controller, so we still don't have the params at this time. The thing to do would be to update the state at onStart, and then re-update it at onSuccess or leave like like that, or clean it at onError. What do you think about that?

ps: that comes from the fact that with ui-router0.3.x and redux-ui-router0.6.0 the @@reduxUiRouter/$stateChangeSuccess action is triggered before the instantiation of the state/controller so the store is updated at the right time 😞

ps2: Another solution rather than changing the store at onStart would be to do it at onFinish which occurs just before the instantiation of the state.