-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
a8d461b
to
12042fd
Compare
@okendoken There is a flash of HTML without CSS on initial render. I was able to reproduce locally |
@goldensunliu agreed, I noticed it too. Looking into it |
@okendoken The same problem when using react-routing |
12042fd
to
bed5f5d
Compare
@goldensunliu @borovik96, updated, please check |
@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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does Redirect work?
There was a problem hiding this comment.
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.
4acb513
to
bed5f5d
Compare
@goldensunliu thanks for your comments 44:13 error `redirectPath` is never modified, use `const` instead prefer-const
46:15 error Missing semicolon semi Could you please run Thanks! |
@okendoken I will get on it |
@okendoken all set flatlogic#2 |
Excited to see this! Thanks for the work everyone :) |
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 |
@jeroenransijn I think you may try to check 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 |
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: @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" |
@erichardson30 the fix was to use 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... |
@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. |
@okendoken where is the latest complete code with the react router so I can see what's wrong? |
@erichardson30 to get latest code with react-router changes you need to run
this will create a new repo with Is there any way I can reproduce this issue locally? |
@okendoken I was able to clone that branch, merge in my changes and everything seems to be good now. Thanks for all your help :) |
+1 - would love to see this merged. |
53f4474
to
5bc1930
Compare
synced with master |
@okendoken Have you done anything using react-router where you implement using isomorphic rendering getting data from an external API? |
Great work, thanks! |
5bc1930
to
2074e5a
Compare
@okendoken hi, you PR is incomplate |
@Bogdaan what's wrong with it? |
@okendoken for example see react-routing conditions in webpack.config.js (find by text match in ide). |
@Bogdaan please see discussion above. In order to allow asynchronous routing on the client side you need to use As of react-routing mentions, I'll remove them and update the PR, thanks for pointing out. |
2074e5a
to
bb57c67
Compare
react-routing mentions removed. Commits rebased |
bb57c67
to
19affa3
Compare
@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 |
@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 |
@okendoken looks like async routes works fine without |
@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. |
1333c2d
to
1785a23
Compare
1785a23
to
e1ea684
Compare
How is the progress? |
@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 |
@okendoken That's shame! |
I'm struggling in doing this on my own these days. I feel this repo is not that active these days. @okendoken |
I'm really waiting for this merge 👍 |
@dhyegofernando just use the fork for now. I don't think it will be merged into this project |
@goldensunliu that's what I'm gonna do but it could be really useful if created in a separated branch just like Thanks for the project. |
It would be great to have the option to use react-router. The react-routing library doesn't meet our needs. |
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. |
@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 |
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