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

Replace universal example with async-with-routing and async-universal #1403

Closed
wants to merge 42 commits into from

Conversation

xulien
Copy link

@xulien xulien commented Feb 15, 2016

hi,

this is my proposal related to reactjs/react-router-redux#232 (comment)

"react-dom": "^0.14.7",
"react-redux": "^4.2.1",
"react-router": "^2.0.0",
"react-router-redux": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use 4.x instead? I think there’s a beta out. It has a slightly different API but it’s better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, it's done

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2016

Hi, thank you for contributing to examples.
I thought about it a bit more, and I’m not sure this is the way we want to go.
(Sorry for the churn!)

I don’t think we want any more counter examples. In fact it would be better to change the existing universal example to do something else. I would say that async example is the best candidate for “universalification”.

I would prefer if we:

  • Add async-with-routing example that was based on async but also used React Router. It would change URLs when the subreddit changes. No need for react-router-redux there.
  • Rename universal to async-universal. Again, no need for react-router-redux there. It should build on top of async-with-routing but show asynchronous data fetching in universal app. The counter example there doesn’t really make sense so we can get rid of it.
  • Keep real-world example as the only example using react-router-redux because it is the only example using DevTools which is what that package is primarily for.

Does this make sense?

@xulien
Copy link
Author

xulien commented Feb 15, 2016

No problem
I'm newcomer into react world, and I'm still learning redux; I haven't read async chapter yet.
But I have some questions in mind, and I don't know if it's already discuss somewhere, or if those are stackoverflow questions.

how to deal with initialState on huge app? all the data store should be loaded on each "url entry"? or should we populate the initialState accordingly to url location on server side and duplicate the router.js? is there already an elegant way to do that?

and finally should I stick on simple declarative router, or using a dynamic router?

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2016

On the server side, you could

  • match the route using match() from React Router
  • use some convention like a static fetchData() on route handlers that returns an async action
  • wait for all async actions to finish dispatching
  • render app with the store that has prefilled data.

You don’t need to worry about initialState argument on createStore(). To prefill it on the server, dispatch actions.

@xulien
Copy link
Author

xulien commented Feb 15, 2016

thank you, I will try to produce something at the earliest

@FarhadG
Copy link

FarhadG commented Feb 17, 2016

I was looking for a simple example using react router with a universal app and I'm glad I stumbled upon this. Thank you, @xulien, and I'm really looking forward to seeing @gaearon's suggestions implemented.

Question:

  • I noticed very slow route changes when I ran this demo with react-router-redux@^3.0.0. I had to upgrade to react-router-redux@>4.0.0-beta. Is there a reason for this?

@xulien
Copy link
Author

xulien commented Feb 17, 2016

I'm not sure if I took the right way to track URL changes, but it's a first proposal to async-router

@xulien
Copy link
Author

xulien commented Feb 17, 2016

@FarhadG my pleasure :)
about your question, I tried both and I haven't seen differences !?

super(props)
this.handleChange = this.handleChange.bind(this)
this.handleRefreshClick = this.handleRefreshClick.bind(this)
this.rootPath = location.pathname.substring(0, location.pathname.lastIndexOf('/') + 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Can’t we just navigate to /stuff?

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2016

Thank you for your work on this! I left a few comments inline.

Please skim through my earlier comment too: #1403 (comment).

Rather than create universal-with-router, I would like to kill the existing universal example and create an async-universal example that combines the two. The current “counter” example in universal doesn’t make sense so I wanted to get rid of it.

Basically async-universal should be = async-with-routing you just did + universal rendering and data fetching on the server.

@xulien
Copy link
Author

xulien commented Feb 17, 2016

I hadn't miss it, but I'm waited review on async-router. async-universal should coming soon

Thanks for review

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2016

Great, thanks! Can you please remove the irrelevant parts for now so it’s easier to review, and get lint to pass? Then I can merge this PR and you can work on the next one.

@xulien
Copy link
Author

xulien commented Apr 12, 2016

@gaearon

hell time ended last week

somebody else can pick it up

it's miiine, my preeecious! and it seems I'm close to finish it.

@xulien
Copy link
Author

xulien commented Apr 12, 2016

I’d say we should write this file in CommonJS for consistency’s sake

if I write server.js in CommonJS, it's the whole async-universal example I must rewrite in CommonJS, and it will not be more consistent !?

@gaearon
Copy link
Contributor

gaearon commented Apr 12, 2016

if I write server.js in CommonJS, it's the whole async-universal example I must rewrite in CommonJS, and it will not be more consistent !?

No, just server.js. Alternatively you can replace server.js of all other examples with the ES6 version. It shouldn’t be much work since they’re pretty much identical.

@xulien
Copy link
Author

xulien commented Apr 13, 2016

I suddenly doubt
When i read "write this file in CommonJS", I understood "don't use babel on server.js".
but i having slept on it, and it seems it's more like "just replace const by var and import by require".

Second purpose is the right one?

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2016

Second purpose is the right one?

Yea, that’s what I meant (just for the sake of consistency).

@xulien
Copy link
Author

xulien commented Apr 14, 2016

argl, sorry for noise

@xulien
Copy link
Author

xulien commented Apr 21, 2016

@gaearon ready for (probably) the last review

@mxstbr
Copy link
Contributor

mxstbr commented Apr 21, 2016

Awesomeee, good work @xulien! Can't wait for this to be merged 🔥

@xulien
Copy link
Author

xulien commented Apr 21, 2016

@mxstbr great thx, but I'll waiting review before to shout victory! ( I'm shaking on each push 😨 )

gaearon added a commit that referenced this pull request Jun 11, 2016
Replace universal example with async-with-routing and async-universal
@gaearon gaearon mentioned this pull request Jun 11, 2016
@xulien
Copy link
Author

xulien commented Jun 29, 2016

this works is in safe hands, then i close this pull request.

@xulien xulien closed this Jun 29, 2016
@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2016

👍 Thanks for your work! Hopefully I’ll get to it soon.

@gaearon gaearon reopened this Aug 7, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 7, 2016

Reopening because I got stuck with #1800. There are still some things that need more work here, I’ll try to post a review when I get more time from other projects.

@timdorr
Copy link
Member

timdorr commented Aug 16, 2016

Reclosing in favor of #1883.

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

Successfully merging this pull request may close these issues.

6 participants