Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(app): redux-simple-router + universal-redux npm package #685

Closed
wants to merge 67 commits into from

Conversation

bdefore
Copy link
Collaborator

@bdefore bdefore commented Dec 10, 2015

Some of you might have seen my universal-redux npm package: https://github.com/bdefore/universal-redux .. there's another PR out for it here: #626. It extracts away a lot of Webpack, Express, and universal rendering from this project, and leaves behind just the parts from the example project itself, plus the API server. But that PR stays with redux-router which the community is starting to move away from in lieu of redux-simple-router.

I've just released a 1.0.0-beta1 version which switches to using redux-simple-router, following along much of the steps of @jlongster 's PR fork: reactjs/react-router-redux#13

What's nice about this is that those who have used this project as a boilerplate who'd like to stay with redux-router can pin to ^0.16 of universal-redux while further development can proceed with using redux-simple-router by requiring ^1.0.0-beta1. It also extracts ApiClient into a more formal redux middleware, which would make exploring redux-fetcher easier to do.

I've deployed it here: http://universal-redux.herokuapp.com

Far as I can see everything seems to work great (Widget, Chat, routing).

@bdefore
Copy link
Collaborator Author

bdefore commented Dec 10, 2015

Related code of universal-redux version 1.0.0-beta1 lives in redux-simple-router branch: https://github.com/bdefore/universal-redux/tree/redux-simple-router

@Ghostavio
Copy link

wasn't this supposed to also fix the redirect loop on IE9?

@bdefore
Copy link
Collaborator Author

bdefore commented Dec 10, 2015

@Ghostavio If you're referring to #416, I believe redux-router 1.0.0-beta5 fixes the issue, but that shouldn't apply with this PR since redux-router has been removed. I noticed that I had a stale push to Heroku and have updated it. Can you try again at https://universal-redux.herokuapp.com ? I tested it and it seemed to function normally on Browserstack running Windows 7 IE9.

@Ghostavio
Copy link

Yes, sorry, it's working just fine, thanks :)

@quicksnap
Copy link
Collaborator

Just a note for posterity: this is a huge change, so it's under heavy scrutiny. I have a project to bootstrap and will give it a spin on there. I'd love to hear any feedback from other people on their experience with the npm module.

I think it's a great idea, but it can open up a new class of problems, sot it'd be great to get as much early feedback as possible. If it works out well, we could maybe cut a different beta version that depends on the module, or some other path.

Also, I wouldn't feel comfortable merging this without discussing with @erikras. I believe he's involved in other matters at the moment.

@bdefore
Copy link
Collaborator Author

bdefore commented Dec 16, 2015

@quicksnap all very sensible. also, i would see this issue as a blocker for now: bdefore/universal-redux#7 i'm currently investigating.

@bdefore
Copy link
Collaborator Author

bdefore commented Dec 20, 2015

@quicksnap all of my 0.x changes are now merged into the 1.x code (on master) ... you should be good to go aside from the issue mentioned above.

@korczis
Copy link
Contributor

korczis commented Dec 20, 2015

Are here any plans to merge this PR? @erikras ?

@catamphetamine
Copy link
Contributor

Since I didn't get which PR shold be commented upon, I'll just copy & paste my comment here too:

Is redux-router really done?
I saw some of the old issues being closed recently.

As for the movement towards an npm package - that's a must.
This project started loosing popularity because it became too complex.
Not a single newcomer will be able to wrap one's mind over all these piles of code.

Where the demarkation line should be drawn is another question.
Obviously noone wan't to know about Webpack weirdness and all the moving parts, that's for sure.
Ideally an average user would like to have a sinlge "DO IT" button to push to start his own project based on this boilerplate.
In this sense this universal-redux thing may be the way to go for the project owners.
It needs to be Rails-alike kind of thing.

@andresgutgon
Copy link

@bdefore how are those changes related with this PR:
#833

Is compatible this code with using async-props?

Thanks for all the hard work. This is great :)

@bdefore
Copy link
Collaborator Author

bdefore commented Jan 16, 2016

@andresgutgon universal-redux takes a very similar approach to that PR. it also supports async-props as of version 3.0.0-rc5.

@andresgutgon
Copy link

Great, sorry I saw it after comment here. It's great your work on universal-redux

@sars
Copy link
Contributor

sars commented Jan 16, 2016

@andresgutgon It's just very fat PR, my is much smaller and i believe it's in the same direction.

@bdefore
Copy link
Collaborator Author

bdefore commented Jan 16, 2016

I'm going to close this PR as it's superseded by #759 Please direct any further comments over there.

@bdefore bdefore closed this Jan 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants