Skip to content
This repository has been archived by the owner on Jan 22, 2020. It is now read-only.

Fix #148: Update deprecated history and RoutingContext for react-router #155

Merged
merged 7 commits into from
May 13, 2016
Merged

Fix #148: Update deprecated history and RoutingContext for react-router #155

merged 7 commits into from
May 13, 2016

Conversation

remarkablemark
Copy link
Contributor

Resolves #148

Tasks:

  • Rename react-router RoutingContext to RouterContext
  • Use react-router's browserHistory instead of history
  • Remove history from lib/client.js and package.json
  • Bump react-router to 2.4.0

Chore:

  • Update README.md with removal of history and specify [email protected] since the latest is version 15.0.2
  • Make sure all tests pass
  • Update .gitignore and ignore *.swp files

- `history` module is no longer needed since history is now
  provided by react-router (browserHistory)
- Remove `history` from dependencies
- And specify the `react` version to be installed at 0.14 since
  the latest is currently at 15.0.2
- In `server.create` method, move `createOptions` to the top of
  the scope since it defaults to an empty object is undefined
@samsel
Copy link
Contributor

samsel commented May 13, 2016

thanks @remarkablemark . would you mind updating the example app as well? - https://github.com/paypal/react-engine/tree/master/example

@samsel samsel merged commit bbc4906 into paypal:master May 13, 2016
@remarkablemark
Copy link
Contributor Author

@samsel, please take a look at #158 and let me know what you think. Thanks!

@@ -40,7 +39,7 @@
"jsdom": "3.0.0",
"jshint": "^2.6.3",
"react": "^0.14.0",
"react-router": "^1.0.3",
"react-router": "^2.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@samsel the react router dependency update broke our app since our React Router code, with a package.json dependency of ~1.0.3, does not work with React Router 2.x. This package.json dependency chain is interesting since React Engine went from working, with a package.json dependency of ^3.3.0, to not working when React Engine update to 3.4. We run npm prune and then npm install, but the issue was not revealed until our build did a clean install of the node packages i.e. you may not experience this issue until node_modules is removed and re-installed. Maybe you want to consider a major version bump with this type of update?

The fix for this issue was to freeze React Router at 1.0.3 in our package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinMGaiam Thanks for letting us know. What were the errors you were getting?

@samsel I just realized that for those still using react-router@1, I may need to update the logic here since the history module is installed separately. What are your thoughts?

Copy link
Contributor

@JustinMGaiam JustinMGaiam May 16, 2016

Choose a reason for hiding this comment

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

@remarkablemark for our code base when we turn on React Router 2.x we end up without any errors and no HTML returned in the res.render callback for the express view. This was something we also tried before the package.json change was made to React Engine with the same result.

Copy link
Contributor

Choose a reason for hiding this comment

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

@remarkablemark that patch should make react-engine seamlessly work with v1.x and 2.x. can you issue a PR?

@JustinMGaiam can you paste the error logs (for the one you encountered before freezing react-router to 1.0.3)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samsel I've added the fix to #159.

I know the original PR was meant to update the example app, so would you like me to cherry-pick the fix and make a separate PR?

Copy link
Contributor

@JustinMGaiam JustinMGaiam May 16, 2016

Choose a reason for hiding this comment

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

@samsel and @remarkablemark here is what I get when I output the err variable for the callback of res.render in express:

Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. at invariant ([project-path-removed]/node_modules/fbjs/lib/invariant.js:39:15) at instantiateReactComponent ([project-path-removed]/node_modules/react/lib/instantiateReactComponent.js:64:134) at [project-path-removed]/node_modules/react/lib/ReactServerRendering.js:41:31 at ReactServerRenderingTransaction.Mixin.perform ([project-path-removed]/node_modules/react/lib/Transaction.js:136:20) at Object.renderToString ([project-path-removed]/node_modules/react/lib/ReactServerRendering.js:40:24) at renderAndDecorate ([project-path-removed]/node_modules/react-engine/lib/server.js:112:30) at reactRouterMatchHandler ([project-path-removed]/node_modules/react-engine/lib/server.js:178:31) at [project-path-removed]/node_modules/react-router/lib/match.js:58:5 at [project-path-removed]/node_modules/react-router/lib/useRoutes.js:120:15 at done ([project-path-removed]/node_modules/react-router/lib/AsyncUtils.js:49:19)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JustinMGaiam Thanks for the error output. I've managed to reproduce it myself in the example app by installing react-router@1.

The error was caused by not having a fallback for react-router v1 when I bumped react-router to v2 in #148.

I've made a fix in this commit. Waiting for @samsel to review it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@remarkablemark thanks for getting this fixed. I've published v3.4.1 with your changes.

@JustinMGaiam can you verify by pulling the latest (npm install)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@samsel this is confirmed working with version React Engine v3.4.1, thanks for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants