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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ddc7221
Rename react-router `RoutingContext` to `RouterContext`
remarkablemark May 10, 2016
c704861
Use `browserHistory` from react-router
remarkablemark May 10, 2016
2290cdd
Remove `history` from client and as a dependency
remarkablemark May 10, 2016
70d7fd5
Ignore swap files
remarkablemark May 10, 2016
7e58422
Update README install instructions with removal of `history`
remarkablemark May 10, 2016
ccff218
Bump `react-router` to latest version 2.4.0
remarkablemark May 10, 2016
86a8c43
Fix failing test in `server.js`
remarkablemark May 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ node_modules/
.idea
.project
.settings
*.swp
logs
*.log
coverage/
Expand Down
5 changes: 2 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@

### Install
```sh
# In your express app, react-engine needs to be installed along
# side react and optionally react-router (+ history, react-router's dependency)
npm install react-engine react react-router history --save
# In your express app, react-engine needs to be installed alongside react (react-router is optional)
$ npm install react-engine [email protected] react-router --save
```

### Usage On Server Side
Expand Down
5 changes: 3 additions & 2 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
'use strict';

var Config = require('./config.json');
var createHistory = require('history').createHistory;
var ReactDOM = require('react-dom');
var merge = require('lodash/object/merge');

Expand Down Expand Up @@ -52,11 +51,13 @@ exports.boot = function boot(options, callback) {
var Router;
var RouterComponent;
var match;
var browserHistory;

try {
Router = require('react-router');
RouterComponent = Router.Router;
match = Router.match;
browserHistory = Router.browserHistory;
} catch (err) {
if (!Router && options.routes) {
throw err;
Expand All @@ -79,7 +80,7 @@ exports.boot = function boot(options, callback) {
throw new Error('asking to use react router for rendering, but no routes are provided');
}

history = options.history || createHistory();
history = options.history || browserHistory;
location = _window.location.pathname +
_window.location.search + _window.location.hash;

Expand Down
8 changes: 4 additions & 4 deletions lib/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,24 @@ function generateReactRouterServerError(type, existingErrorObj, additionalProper
}

exports.create = function create(createOptions) {
createOptions = createOptions || {};

// safely require the peer-dependencies
var React = util.safeRequire('react');
var Router;
var match;
var RoutingContext;
var RouterContext;

try {
Router = require('react-router');
match = Router.match;
RoutingContext = Router.RoutingContext;
RouterContext = Router.RouterContext;
} catch (err) {
if (!Router && createOptions.routes) {
throw err;
}
}

createOptions = createOptions || {};
createOptions.docType = isString(createOptions.docType) ? createOptions.docType : Config.docType;
createOptions.renderOptionsKeysToFilter = createOptions.renderOptionsKeysToFilter || [];

Expand Down Expand Up @@ -175,7 +175,7 @@ exports.create = function create(createOptions) {
return React.createElement(Component, merge({}, data, routerProps));
};

return done(null, renderAndDecorate(React.createElement(RoutingContext, renderProps), data, html));
return done(null, renderAndDecorate(React.createElement(RouterContext, renderProps), data, html));
} else {
debug('server.js match 404');
var err = generateReactRouterServerError(ReactRouterServerErrors.MATCH_NOT_FOUND);
Expand Down
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
"dependencies": {
"debug": "^2.1.3",
"glob": "^5.0.3",
"history": "^1.12.2",
"lodash": "^3.10.1",
"parent-require": "^1.0.0",
"react-dom": "^0.14.0"
Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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!

"rewire": "^2.3.1",
"sinon": "^1.14.1",
"tape": "^3.5.0",
Expand Down