-
Notifications
You must be signed in to change notification settings - Fork 297
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
[universal-redux] Upgrading to react-router 4.x #195
[universal-redux] Upgrading to react-router 4.x #195
Conversation
Codecov Report
@@ Coverage Diff @@
## universal-redux #195 +/- ##
===============================================
Coverage 100% 100%
===============================================
Files 9 75 +66
Lines 308 732 +424
Branches 92 171 +79
===============================================
+ Hits 308 732 +424
Continue to review full report at Codecov.
|
Awesome, @kybarg Will review that asap |
package.json
Outdated
@@ -49,11 +49,11 @@ | |||
"nodemon": "^1.11.0", | |||
"opn-cli": "^3.1.0", | |||
"react-addons-test-utils": "^15.4.1", | |||
"react-hot-loader": "^3.0.0-beta.6", |
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.
react-hot-loader
needs to be a production dependency. The library itself handles production
/development
properly.
package.json
Outdated
"cross-env": "^3.1.4", | ||
"csurf": "^1.9.0", | ||
"express": "^4.14.0", | ||
"express-force-ssl": "^0.3.2", | ||
"file-loader": "^0.10.0", | ||
"history": "3.0.0", | ||
"history": "^4.6.1", | ||
"isomorphic-fetch": "^2.2.1", |
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.
Could you please make that change (replacing axios
by isomorphic-fetch
) on another PR? It's actually a bug we missed.
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.
@diegohaz done
src-clean/client.js
Outdated
const baseHistory = useRouterHistory(createHistory)({ basename }) | ||
const store = configureStore(initialState, baseHistory) | ||
const history = syncHistoryWithStore(baseHistory, store) | ||
const history = createHistory() |
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.
What about basename
?
component && | ||
component[method] && | ||
promises.push(component[method]({ req, res, params, location, store })) | ||
const fetchData = () => new Promise((resolve, reject) => { |
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.
How does fetchData
work now? Is that still asynchronous?
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.
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.
@protoEvangelion Can't reproduce on test environment. Could you please provide more details?
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.
@diegohaz Sure, nothing changed in general. Only react-router
match logic has changed.
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.
@kybarg Sorry for the delay! This was not an issue with the upgrade. The problem existed before the upgrade. It has to do with the front end adding identical keys when creating a new post. I will address this after React-Router-4 is merged :)
Thank you, @kybarg ❤️ Will review that this weekend. |
How'd the review go @diegohaz |
@diegohaz Any updates or your are closing this branch in favor for |
I still didn't find time to proper review that. Will do as soon as possible. |
Can you delegate anyone else to review this? Seems like you don't have much time to, would prefer quicker updates |
Thanks for keeping this branch up to date @kybarg |
@Geczy You are welcome 😄 |
@kybarg can you merge the latest redux branch? |
Running
|
@Geczy |
|
what's the word @kybarg |
I'm looking into this right now. Brace yourselves, merge is coming. |
Sorry for the delay and thank you very much, @kybarg and everyone. I fixed the problem with pages in storybook. The current |
closes #161