Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Redux-simple-router and React-Router replaceState interactions #69

Closed
ingro opened this issue Dec 3, 2015 · 15 comments
Closed

Redux-simple-router and React-Router replaceState interactions #69

ingro opened this issue Dec 3, 2015 · 15 comments

Comments

@ingro
Copy link
Contributor

ingro commented Dec 3, 2015

Hello! I need to implement authentication mechanics to my application and so instead of starting from scratch I've used React-Router auth-flow example as example.

I'm facing a strange problem: from the Login component (roughly the same as the example above) I should be able to redirect to the previous visited route if auth is successfull:

if (loggedIn) {
   const { location, history } = this.props;

   if (location.state && location.state.nextPathname) {
       history.replaceState(null, location.state.nextPathname);
   } else {
       history.replaceState(null, '/');
   }
}

This path should be stored inside the location state object by the onEnter hook on a protected route:

function requireAuth(nextState, replaceState) {
    if (! isLoggedIn()) {
        replaceState({ nextPathname: nextState.location.pathname }, '/login');
    }
}

If I use this logic with redux-simple-route, inside the Login component the props this.location.state is null, while, if disabled, contains the right nextPathname property.

I think I've narrowed down the problem to these lines of redux-simple-router (68-71):

if(lastChangeId !== routing.changeId) {
  lastChangeId = routing.changeId;
  history.pushState(null, routing.path);
}

It pushes the route to history (which it should be not necessary) and doesn't pass the state in the process.

A simple fix would be setting changeId in the initial state to 0 but I don't know if it may cause other things to stop working...

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 3, 2015

This should be solved in master. There's been quite a few commits lately, so hopefully we'll see a new version soon.

Btw, while waiting for a new release I depend on master instead of 0.0.10:

"redux-simple-router": "https://github.com/jlongster/redux-simple-router.git#master",

@ingro
Copy link
Contributor Author

ingro commented Dec 3, 2015

Thanks I will try with master so!

@gavacho
Copy link

gavacho commented Dec 4, 2015

When I update my package.json like that, webpack starts to fail:

ERROR in ./app/main.js
Module not found: Error: Cannot resolve module 'redux-simple-router' in /Users/ken/heroproof/app
 @ ./app/main.js 79:25-55

When I look at the package.json in node_modules/redux-simple-router I see "main": "lib/index" but I don't see any directory lib.

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 4, 2015

Ah, my bad, I totally forgot it must be built :/ Sorry. You can fix this with:

cd node_modules/redux-simple-router
npm install
npm run build

So not exactly very nice, but it helps if you want to test it.

@jlongster I think it's about time with a new release. Lots of new stuff in master right now (and I really should stop telling people to use master — it's not a very good recommendation :/)

@gavacho
Copy link

gavacho commented Dec 4, 2015

Thanks for responding, @kjbekkelund. Count me among those eager for a new release.

When I build in-place per you instructions, I end up with a file at lib/index.js, so that's good. Unfortunately the file is still es6. It's not identical to src/index.js, but it's still got all the const and =>. I believe the file at lib/index.js is intended to be es5.

@gavacho
Copy link

gavacho commented Dec 4, 2015

Sorry, I figured out that npm run build was calling the globally installed babel and I guess mine was out of date. When I change the command to mkdir -p lib && node_modules/.bin/babel ./src/index.js --out-file ./lib/index.js, it works. Thanks!

@gavacho
Copy link

gavacho commented Dec 4, 2015

I spoke too soon, that didn't fix it.

@gavacho
Copy link

gavacho commented Dec 4, 2015

It was the missing .babelrc file.

@jlongster
Copy link
Member

Why does it need to be built? Does installing from a github repo not run the post-install hooks?

@jlongster
Copy link
Member

@jlongster I think it's about time with a new release. Lots of new stuff in master right now (and I really should stop telling people to use master — it's not a very good recommendation :/)

Agreed. I really wanted to fix the devtools issue, and there were a few coding style tweaks I was going to do. I personally haven't been update to play with master yet.

I can work on it in the next day or two. I really wanted to to at least look at the devtools issue.

@gavacho
Copy link

gavacho commented Dec 4, 2015

@jlongster I think it's because the build is setup to occur during the prepublish hook

@jlongster
Copy link
Member

Ah right. I wonder if there's a way to get it working from a straight git install.

@jlongster
Copy link
Member

Can you try master? I'm going to close this this week unless you can reproduce the issue.

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 11, 2015

Closed this, let us know if you still have problems

@ingro
Copy link
Contributor Author

ingro commented Dec 12, 2015

Sorry for the late reply, anyway 1.0 fixed my problem, thanks for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants