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

Use react-router; client- and server side rendering #422

Closed
wants to merge 2 commits into from

Conversation

okendoken
Copy link

Use react-router instead of react-routing library.

As per #345

Update. Here is a live version with react-router https://limitless-wave-36322.herokuapp.com/


This change is Reviewable

@goldensunliu
Copy link

@okendoken There is a flash of HTML without CSS on initial render. I was able to reproduce locally

@okendoken
Copy link
Author

@goldensunliu agreed, I noticed it too. Looking into it

@vbrvk
Copy link

vbrvk commented Jan 31, 2016

@okendoken The same problem when using react-routing

@okendoken
Copy link
Author

@goldensunliu @borovik96, updated, please check

@goldensunliu
Copy link

@okendoken just curious what was the fix is since I still see it locally.

await Router.dispatch({ path: req.path, query: req.query, context }, (state, component) => {
data.body = ReactDOM.renderToString(component);
match({ routes, location: req.url }, (error, redirectLocation, renderProps) => {
let statusCode = 200;

Choose a reason for hiding this comment

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

does Redirect work?

Choose a reason for hiding this comment

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

@okendoken I added redirect and error handling logic here: goldensunliu@d6885b9 just tested.

@okendoken
Copy link
Author

@goldensunliu thanks for your comments
I tried to merge your commit and it seems that there are errors you need to fix:

  44:13  error  `redirectPath` is never modified, use `const` instead  prefer-const
  46:15  error  Missing semicolon                                      semi

Could you please run npm run test, check for errors, fix them and make another commit (preferably squashing your commits into a single one)? It would be also nice if you force update your fork with last changes from mine - we have different commits history. Otherwise I will have to cherry-pick.

Thanks!

@goldensunliu
Copy link

@okendoken I will get on it

@goldensunliu
Copy link

@okendoken all set flatlogic#2

@jeroenransijn
Copy link

Excited to see this! Thanks for the work everyone :)

@jeroenransijn
Copy link

I am trying out this fork/branch. I am trying to hook it up to redux, since that's what I am using. The issue that comes up is when I try to connect my component export default connect(state => state)(withStyles(Navigation, s));. The server just keeps loading, it get stuck on ReactDOM.renderToString. It's really difficult to debug this since nothing is returning. Any help is appreciated.

@frenzzy frenzzy mentioned this pull request Feb 4, 2016
@okendoken
Copy link
Author

@jeroenransijn I think you may try to check getContextComponent function https://github.com/kriasoft/react-starter-kit/pull/422/files#diff-3ca5e313cdf02368ce63986a36207c94R20

Sometimes I observed the same behavior when there was no page found during component lookup. We probably need to rethrow the error from the inside of this function

@erichardson30
Copy link

I added the changes from master...flatlogic:feature/react-router and am getting the following warning : Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server:
(client) <noscript data-reacti
(server) <div data-reactid=".c

@okendoken I saw you asked about this initially...what was the fix for this?

If I make the server.js and client.js render methods match....then I get "Get http://localhost:5000/main.js not found"

@okendoken
Copy link
Author

@erichardson30 the fix was to use match function on the client side also.

But where do you get it? I can't see any issues. I have just redeployed herokuapp - https://limitless-wave-36322.herokuapp.com/. See nothing there...

@erichardson30
Copy link

@okendoken So when you run npm start, it tells you the server is running on localhost:5000 and it opens up the client on localhost:3000. The warning is being thrown when I access through localhost:3000. I went through https://github.com/kriasoft/react-starter-kit/pull/422/files#diff-3ca5e313cdf02368ce63986a36207c94R20 and copied all replaced all the files in my project with those.

@erichardson30
Copy link

@okendoken where is the latest complete code with the react router so I can see what's wrong?

@okendoken
Copy link
Author

@erichardson30 to get latest code with react-router changes you need to run

git clone https://github.com/flatlogic/react-starter-kit.git -b feature/react-router

this will create a new repo with feacture/react-router branch

Is there any way I can reproduce this issue locally?

@erichardson30
Copy link

@okendoken I was able to clone that branch, merge in my changes and everything seems to be good now. Thanks for all your help :)

@learntoswim
Copy link

+1 - would love to see this merged.

@okendoken
Copy link
Author

synced with master

@erichardson30
Copy link

@okendoken Have you done anything using react-router where you implement using isomorphic rendering getting data from an external API?

@Bogdaan
Copy link

Bogdaan commented Feb 17, 2016

Great work, thanks!
If someone needs case authorization,
i merge this PR with some changes to my repo (https://github.com/Bogdaan/react-auth-kit/blob/master/src/server.js)

@Bogdaan
Copy link

Bogdaan commented Feb 20, 2016

@okendoken hi, you PR is incomplate
See https://github.com/flatlogic/react-starter-kit/blob/feature/react-router/tools/webpack.config.js
and client-side matching is incorrect

@okendoken
Copy link
Author

@Bogdaan what's wrong with it?

@Bogdaan
Copy link

Bogdaan commented Feb 20, 2016

@okendoken for example see react-routing conditions in webpack.config.js (find by text match in ide).
In client.js at (https://github.com/flatlogic/react-starter-kit/blob/feature/react-router/src/client.js#L62)
Why are you use window.location and match on client-side?

@okendoken
Copy link
Author

@Bogdaan please see discussion above. In order to allow asynchronous routing on the client side you need to use match as well remix-run/react-router#2714.

As of react-routing mentions, I'll remove them and update the PR, thanks for pointing out.

@okendoken
Copy link
Author

react-routing mentions removed. Commits rebased

@okendoken okendoken force-pushed the feature/react-router branch 3 times, most recently from bb57c67 to 19affa3 Compare February 22, 2016 16:09
@okendoken
Copy link
Author

@koistya what's your feedback on this PR? It's been here for 25 days. Are you going to merge it? Please provide a resolution, as it's a bit annoying to keep the PR in sync constantly merging changes from master. Thanks

@koistya
Copy link
Member

koistya commented Feb 22, 2016

@okendoken thanks for the PR! I'm currently working on the authentication feature, which should be done within next couple of days. Maybe I can take it over and do rebase and some modifications myself until it's ready for master branch? If you like, you can give me access to your fork, or I can give you write access to my fork.

@Bogdaan
Copy link

Bogdaan commented Feb 22, 2016

@okendoken looks like async routes works fine without render() ( see remix-run/react-router#2883 )

@okendoken
Copy link
Author

@koistya thanks for your answer! Your timeline looks fine - I'll keep the fork in sync then. I was afraid this PR went unnoticed at all ;)

Just in case, I added you as a collaborator to our repo.

@okendoken okendoken force-pushed the feature/react-router branch 2 times, most recently from 1333c2d to 1785a23 Compare February 29, 2016 15:20
@Stupidism
Copy link

How is the progress?

@okendoken
Copy link
Author

@stupidisum the code https://github.com/flatlogic/react-starter-kit/tree/feature/react-router was completely ready more than two month ago and we kept it in sync with react-starter-kit master branch for the whole month and asked project creator @koistya to provide a resolution on this PR. However, it seems that they are not really interested in introducing react-router to this repo and we won't be able to spend more resources on keeping this ignored PR in sync.

Feel free to use our repo and, if you like, keep it in sync with react-starter-kit.

Thanks!

Refs #160

@learntoswim
Copy link

@okendoken That's shame!

@Stupidism
Copy link

I'm struggling in doing this on my own these days.
This pr is OK for now.
But once authentication is added. Boom! Server-render problem again.
And today, I found this and they've been much more further than react-starter-kit.

I feel this repo is not that active these days. @okendoken

@dhyegocalota
Copy link

I'm really waiting for this merge 👍

@goldensunliu
Copy link

@dhyegofernando just use the fork for now. I don't think it will be merged into this project

@dhyegocalota
Copy link

dhyegocalota commented May 31, 2016

@goldensunliu that's what I'm gonna do but it could be really useful if created in a separated branch just like feature/redux, like our friend @okendoken did... what do you think @koistya? Everybody is excited with that.

Thanks for the project.

@gardner
Copy link

gardner commented Jul 21, 2016

It would be great to have the option to use react-router. The react-routing library doesn't meet our needs.

@okendoken
Copy link
Author

A small update to this pr:

We forked RSK and created react-dashboard. It has react-router & bootstrap integrated. We plan to continuously maintain it and keep it in sync with RSK.

@ulani
Copy link
Member

ulani commented May 27, 2021

@okendoken thank you very much for this PR! Unfortunately, we have close it due to inactivity. Feel free to re-open it or join our Discord channel for discussion.

NOTE: The main branch has been updated with React Starter Kit v2, using JAM-style architecture.

@ulani ulani closed this May 27, 2021
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.

None yet