Skip to content

fetchData() and fetchDataDeferred() behave as they should #4

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
"express": "^4.13.3",
"express-session": "^1.12.1",
"file-loader": "^0.8.4",
"history": "^1.13.0",
"history": "^1.13.1",
"hoist-non-react-statics": "^1.0.3",
"http-proxy": "^1.12.0",
"less": "^2.5.3",
Expand All @@ -105,11 +105,12 @@
"react-dom": "^0.14.1",
"react-inline-css": "^2.0.0",
"react-redux": "^4.0.0",
"react-router": "^1.0.0-rc3",
"react-router": "^1.0.0-rc4",
"react-router-bootstrap": "^0.19.3",
"redux": "^3.0.4",
"redux-form": "^3.0.0",
"redux-simple-router": "^0.0.7",
"redux-simple-router": "0.0.10",
"scroll-behavior": "^0.3.0",
"serialize-javascript": "^1.1.2",
"serve-favicon": "^2.3.0",
"serve-static": "^1.10.0",
Expand Down
67 changes: 55 additions & 12 deletions src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ import createStore from './redux/create';
import ApiClient from './helpers/ApiClient';
import io from 'socket.io-client';
import {Provider} from 'react-redux';
import {Router} from 'react-router';
import {Router, match, createRoutes} from 'react-router';
import {syncReduxAndRouter} from 'redux-simple-router';
import getRoutes from './routes';
import {fetchData, fetchDataDeferred} from './helpers/fetchComponentsData';
import useScroll from 'scroll-behavior/lib/useStandardScroll';

const client = new ApiClient();

const dest = document.getElementById('content');
const store = createStore(client, window.__data);
const history = createHistory();

syncReduxAndRouter(history, store);
const routes = getRoutes(store);

function initSocket() {
const socket = io('', {path: '/api/ws', transports: ['polling']});
Expand All @@ -36,17 +36,60 @@ function initSocket() {

global.socket = initSocket();

function createElement(Component, props) {
if (Component.fetchData) {
Component.fetchData(store.getState, store.dispatch,
props.location, props.params);
const history = useScroll(createHistory)({routes: createRoutes(routes)});
syncReduxAndRouter(history, store);

let lastMatchedLocBefore;
let lastMatchedLocAfter;

history.listenBefore((location, callback) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you should be using match browser-side at all. We should be using a history that has been augmented with useRoutes: https://github.com/rackt/react-router/blob/master/docs/API.md#useroutescreatehistory

In fact you pass routes into createHistory but that's not doing anything. You need to also wrap createHistory with useRoutes, and then the listen signature changes: you get (error, nextState) => {} instead, and you can just use nextState to get all the components.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, but this is mainly a work-around for this: remix-run/react-router#2502
I don't think there is any practical drawback in using match here, but probably I'm missing something?
Besides, useRoutes can be applied only to listen and not to listenBefore, so unless we find another way for pulling the route components in listenBefore (e.g. react-router gets a patch), this is the only code I got working.

const loc = location.pathname + location.search + location.hash;
if (lastMatchedLocBefore === loc) {
return callback();
}
return React.createElement(Component, props);
}

match({routes: routes, location: loc}, (err, redirectLocation, nextState) => {
if (!err && nextState) {
fetchData(nextState.components, store.getState, store.dispatch,
location, nextState.params)
.then(() => {
lastMatchedLocBefore = loc;
callback();
})
.catch(err2 => {
console.error(err2, 'Error while fetching data');
callback();
});
} else {
console.log('Location "%s" did not match any routes (listenBefore)', loc);
callback();
}
});
});

history.listen((location) => {
const loc = location.pathname + location.search + location.hash;
if (lastMatchedLocAfter === loc) {
return;
}

match({routes: routes, location: loc}, (err, redirectLocation, nextState) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, in fact for this case I think you just need to use the createElement method that was in the previous code and remove this history.listen subscriber entirely.

Copy link
Author

Choose a reason for hiding this comment

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

Using createElement will lead to this: #2). Personally, I found it unusable in my project because of the issue above.
Also, I didn't check, but I'm not sure if multiple pushState (even on the same URL) will cause createElement to be called multiple times. If that's the case we are nullifying the advantages of your new routing logic. Note: in the code above I explicitly disabled this feature (by comparing old and new location), because I don't think it's needed for this particular use case.

if (err) {
console.error(err, 'Error while matching route (change handler)');
} else if (nextState) {
fetchDataDeferred(nextState.components, store.getState,
store.dispatch, location, nextState.params)
.then(() => lastMatchedLocAfter = loc)
.catch((err2) => console.error(err2, 'Error while fetching deferred data'));
} else {
console.log('Location "%s" did not match any routes (listen)', loc);
}
});
});

const component = (
<Router createElement={createElement} history={history}>
{getRoutes(store)}
<Router history={history}>
{routes}
</Router>
);

Expand Down
32 changes: 0 additions & 32 deletions src/helpers/fetchAllData.js

This file was deleted.

24 changes: 24 additions & 0 deletions src/helpers/fetchComponentsData.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@

function _fetchData(components, getState, dispatch, location, params, deferred) {
const methodName = deferred ? 'fetchDataDeferred' : 'fetchData';
return components
.filter((component) => component && component[methodName]) // only look at ones with a static fetchData()
.map((component) => component[methodName]) // pull out fetch data methods
.map(fetchDataFoo =>
fetchDataFoo(getState, dispatch, location, params)); // call fetch data methods and save promises
}


export function fetchData(components, getState, dispatch, location, params) {
return Promise.all(_fetchData(components, getState, dispatch, location, params));
}

export function fetchDataDeferred(components, getState, dispatch, location, params) {
return Promise.all(_fetchData(components, getState, dispatch, location, params, true));
}

export function fetchAllData(components, getState, dispatch, location, params) {
return fetchData(components, getState, dispatch, location, params)
.then(() => fetchDataDeferred(components, getState, dispatch, location, params))
.catch(error => console.error(error, 'Error while fetching data'));
}
2 changes: 1 addition & 1 deletion src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {Provider} from 'react-redux';
import qs from 'query-string';
import getRoutes from './routes';
import getStatusFromRoutes from './helpers/getStatusFromRoutes';
import fetchAllData from './helpers/fetchAllData';
import {fetchAllData} from './helpers/fetchComponentsData';

const pretty = new PrettyError();
const app = new Express();
Expand Down