-
-
Notifications
You must be signed in to change notification settings - Fork 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
[API Proposal] introducing END #255
Comments
I love the approach here. I have one concern:
A general approach in server rendering with co-located data loading is that, when navigating to a specific route, we fire off data fetching. Performing these types of actions twice is far from optimal. I'm sure there are ways to address the double-render symptom and check for it in various places, but preventing it altogether seems important to me. Could we instead utilize |
A simple mechanism to fire data fetching without rendering could be to define a static hook on your React components that returns an action requesting sagas to fetch data. import { loadCat } from './api';
class CatProfile extends React.Component {
static fetchInitialData(params, getState) {
return loadCat({ id: params.catId })
}
componentDidMount() {
const { dispatch, cat, params } = this.props
if (!cat.isLoaded) {
dispatch(CatProfile.fetchInitialData(params))
}
}
render() {
const { cat } = this.props
if (!cat.isLoaded) {
return <p>Fetching cat...</p>
}
return (
<div>
<h1>{cat.name}</h1>
<p>Fur color: {cat.color}</p>
</div>
)
}
}
export default connect(state => {cat: state.cat})(CatProfile); The server could then inspect which components are due to be rendered on match({routes, location: req.url}, (error, redirectLocation, renderProps) => {
if (error) { ... }
else if (redirectLocation) { ... }
else if (renderProps && renderProps.components) {
const rootTask = sagaMiddleware.run(rootSaga)
// this will cause the universal Saga tasks to trigger
for (let component of renderProps.components) {
store.dispatch(component.fetchInitialData(renderProps.params, store.getState.bind(store)))
}
// notify Saga that there will be no more dispatches
// this will break the while loop of watchers
store.dispatch(END)
rootTask.done.then(() => {
res.status(200).send(
Layout(
renderToString(
<Root store={store} renderProps={renderProps} type="server"/>
),
JSON.stringify(store.getState())
)
)
}).catch(...)
} else {
res.status(404).send('Not found')
}
})
}) As far as I can tell this doesn't have any big problems, but then again I've never worked on a production app before, so forgive me if there is a glaring issue staring me right in the face. |
@quicksnap The second render won't trigger any async actions; because i'll occur after dispatching END which causes the Sagas to terminate. So the cost will be only of building the Component tree + markup generation |
I think the cost of double rendering on the server will probably be negligent, and the proposal seems straightforward enough. Automatic handling of I'm not sure how having both would work. @MarcoThePoro I'm not sure if that would work in all cases. For example I have a component that fetches data dependent on the |
I actually think the double rendering is really elegant since you don't need to make modifications to your components to make them universal, plus a static data fetching hook doesn't have access to props so you can't fetch data based on props. I also think automatic |
I also think that For example, let's say I'm taking from a given channel and I'd like to start a task when that channel is closed (e.g. the underlying event source has terminated) function* saga() {
const chan = yield call(eventChannel, eventSource)
let event = yield take(chan)
while(event !== END) {
// do some stuff
event = yield take(chan)
}
// eventSource ended, start another event source
const chan2 = yield call(eventChannel, eventSource2)
// loop over chan2...
} As an FP analogy, it's like instead of having a return type of |
If |
That would be a practical solution. OTOH this also means more surcharging for the catch block, as we already have to perform an isCancel now to discern normal errors from cancellations, we'll also have to perform an isEnd.
Yes. that's on drawback of manual END handling, since we now have a return type of |
IMO checking the type of error is preferable because I do not have many try/catch blocks, but I do have many Although I'm not trying to claim that I know enough to make a well-reasoned decision, just giving my 2 cents. |
@axelson I think the issue you raised makes plain sense. END will be likely used in the server to end the sagas most if the time. So enforcing a check on every take is indeed awkward. It's just it seems weird to use the exception system to raise an exception that wont propagate if not handled. I m thinking of this
|
FYI 0.10.0 is out ended up with the double method of above: cf. release notes |
Just found this, and want to say 👏 . The double-render thing works very well (same method here: https://github.com/ericclemmons/react-resolver), and it's great that this project can solve SSR + Redux. |
Where is |
you can import it like that although it should probably be better documented |
Hi guys, ( @yelouafi, @Andarist ) So do we need to do this now: function* taker() {
let action = yield take(TEST_ACTION);
while (action !== END) { //do we have to check for END?
action = yield take(TEST_ACTION);
}
} function* watcher() {
let action = yield take('*');
while (action !== END) { //do we have to check for END?
const action = yield take('*');
const state = yield select();
console.log('action', action);
console.log('state after', state);
}
} or this will work: function* taker() {
//no need for END check
while(true){
let action = yield take(TEST_ACTION);
console.log('action:',action);
}
} function* watcher() {
//no need for END check
while (true) {
const action = yield take('*');
const state = yield select();
console.log('action', action);
console.log('state after', state);
}
} |
There is no need to check for |
You would also have to share how do u plan to integrate redux-saga into ur project. You could similarly expose relevant sagas on the components, gather them like you have gathered promises, then run them and render once in |
Hmm that's kinda the thing, I am not too familiar with saga so that's why I'm exploring a bit. I thought saga's were only triggered by other actions. Is there any documentation I can find about the stuff you mentioned? |
Dont rly think so, server side story is not well documented. Sagas can be resumed (is they are paused at a listener 'step' - If you associate saga with a component, and that saga start fetching immediately you could do pretty much what I have described. I imagine it could look roughly like this: app/components/App.jsximport React from 'react';
import { connect } from 'react-redux';
// getStuff returns a function that returns a promise, just like redux-thunk
import { getStuffSaga } from 'app/sagas';
export class App extends React.PureComponent {
static fetchData() {
return getStuffSaga;
}
componentDidMount() {
// get sagaMiddleware from the context or smth
sagaMiddleware.run(getStuffSaga)
}
render() {
// ...
}
} server/index.jsimport express from 'express';
import React from 'react';
import { Provider } from 'react-redux';
import { match, RouterContext } from 'react-router';
import { createStore, applyMiddleware } from 'redux';
import createSagaMiddleware, { END } from 'redux-saga';
// reducer is not relevant here
import reducer from 'app/reducers';
import routes from 'app/routes';
const app = express();
app.get('*', (req, res, next) => {
const sagaMiddleware = createSagaMiddleware()
const store = createStore(reducer, , applyMiddleware(sagaMiddleware));
match({ location: req.url, routes }, (err, redirect, props) => {
// handle err, redirect or no props
const render = () => {
res.send(renderToString(
<Provider store={store}>
<RouterContext {...props} />
</Provider>
));
};
// this is the important part: since react router exposes which components it will render
// I am able to go through them and find the ones that have a data dependency
const sagas = props.components
.filter(component => !!component.fetchData)
.map(component => sagaMiddleware.run(component.fetchData, props).done);
if (!sagas.length) {
return render();
}
store.dispatch(END)
Promise.all(sagas)
.then(render)
.catch(next);
});
});
app.listen(3000); |
Ah okay, that looks pretty good. And actually in the |
@chielkunkels how did you go with this? Any luck implementing an elegant solution? I'm looking at getting this working for my server-side side effects. |
@EdiSch I hwven't had a chance to look more into it I'm afraid, been busy with other stuff. I will be getting back to this soon, however. |
Am I doing this entirely wrong? Still trying to get the hang of sagas. I have a universal app and I want one saga to // saga1.js
function *doFetch() {
yield [do some async stuff...]
yield put({type: 'fetchSuccess'})
}
function* saga1() {
yield takeEvery('fetch', doFetch)
}
// saga2.js
function* doFetchSuccess() {
console.log('never got here?')
}
function* saga2() {
yield takeEvery('fetchSuccess', doFetchSuccess)
}
// server.js
sagaMiddleware.run(function* () {
yield [
fork(saga1),
fork(saga2),
]
).done.then(() => {
console.log('done');
})
store.dispatch({type: 'fetch'});
store.dispatch(END); From what I understand from this thread about forks this should be possible but I'm not sure how to set it up. Thanks for any help! |
|
@Andarist so same goes for sagas paused on If so my more general question is, is there a way to set it up such that I can |
Your sagas shouldnt be paused on If you want better control over the However in general it should be possible to restructure your logic in such a way that this is not needed.
It certainly is, however not always on the server side. |
Thank you for a solution. What if the one saga should wait for another one. For example:
How would this scenario be implemented using END? |
No, its not possible - not through |
@keithnorm - Did you end up solving your problem with server-side rendering? What was the solution?
I'm still learning // 1. start: rootSaga
// 2. fork: saga1
// 3. fork: saga2
// 4. dispatch: FETCH_DATA_FROM_SAGA_1
// 5. dispatch: END
// 6. saga2: END - terminates since there is no pending work being done
// 7. saga1: fetches async data
// 8. saga1: put FETCH_DATA_FROM_SAGA_2 (nothing happens since this saga2 has already ended)
// 9. saga1: END - terminates since work is complete I see there was some suggestion to use /cc @Andarist |
@bmealhouse No I didn't. I solved what I was doing differently without needing that ability. I intend to dig into the code and see if I can figure out a way to make this work though because it would make my implementation cleaner and it just feels like it should work, but I'm not yet that familiar with how sagas work under the hood. Let me know if you make any progress on it. |
@Andarist :-( could you please provide a simple example? I think this is a common case. |
@keithnorm @rosendi - I did find a solution to this problem using channels, however, I'm not sure it is the recommended approach. It would be nice to get some feedback from the community, @yelouafi, and @Andarist. Here is a simple weather example. To kick things off, main.js import {END} from 'redux-saga'
import store from './store'
store.dispatch({
type: 'FETCH_LOCATIONS_REQUEST'
})
store.dispatch(END) root-saga.js import {all, fork} from 'redux-saga/effects'
import locationsSaga from './locations-saga'
import weatherSaga from './weather-saga'
function* rootSaga() {
yield all([
fork(locationsSaga),
fork(weatherSaga)
])
}
export default rootSaga locations-saga.js import {channel} from 'redux-saga'
import {call, fork, put, take} from 'redux-saga/effects'
import {weatherChannelHandler} from './weather-saga'
import * as api from './utils/api'
function* fetchLocations() {
const locations = yield call(api.fetchLocations)
yield put({
type: 'FETCH_LOCATIONS_SUCCESS',
locations
})
return locations
}
function* locationsSaga() {
const weatherChannel = yield call(channel)
yield fork(weatherChannelHandler, weatherChannel)
while(true) {
yield take('FETCH_LOCATIONS_REQUEST')
const locations = yield call(fetchLocations)
yield put(weatherChannel, locations)
}
}
export default locationsSaga weather-saga.js function* fetchWeather(action) {
// fetch weather data from api...
// action.locations available here
}
export function* weatherChannelHandler(channel) {
const action = yield take(channel)
yield call(fetchWeather, action) // blocking
}
function* weatherSaga() {
// listen for other actions...
yield takeEvery('SELECT_LOCATION', fetchWeather)
}
export default weatherSaga |
I did it with static methods: https://github.com/suciuvlad/react-universal-starter |
@suciuvlad - Thank's for sharing, however, I don't believe your example is showcasing the problem being discussed. Please correct me if I missed something. In your example, all the asynchronous work is happening inside a single saga. This doesn't cause any issues for server-side rendering, since the server will wait until the saga has completed it's work before sending a response to the client. The problem being discussed, is dealing with two sagas, with one being dependent on the other. |
@bmealhouse Sorry, my bad. |
@suciuvlad - No problem, thanks again for sharing. |
@bmealhouse any idea how to approach using 1st async response data for 2nd async (via seperate actions), both before |
@craigtaub - The example does use data from the 1st async response for the 2nd async request. You can pass any sort of action or raw data into your channel. I hope this helps. function* fetchLocations(weatherChannel) {
const locations = yield call(api.fetchLocations)
yield put({
type: 'FETCH_LOCATIONS_SUCCESS',
locations
})
return locations
}
function* locationsSaga() {
const weatherChannel = yield call(channel)
yield fork(weatherChannelHandler, weatherChannel)
while(true) {
yield take('FETCH_LOCATIONS_REQUEST')
const locations = yield call(fetchLocations)
// trigger second async request with different action
yield put(weatherChannel, {
type: 'DIFFERENT_ACTION',
locations
})
}
} |
Great thanks @bmealhouse . try {
while(true) {
yield take('FETCH_LOCATIONS_REQUEST')
const locations = yield call(fetchLocations)
yield put(weatherChannel, {
type: 'DIFFERENT_ACTION',
locations
})
}
} finally { // END triggered
weatherChannel.close(); // close + unsubscribe from channel
} |
Besides usage with channels (see #254),
END
can also be used with store actions. This would allow us to break thewhile(true)
loop inside watchers. this combined with #78 (support for attached forks) would allow us to write universal Saga code.END
is a special action, when there is saga waiting on a takeif
END
is dispatched, then the take will be resolved no matter what SOME_ACTION is. Similarly, if the store/channel alreadyEND
ed and a Saga emits atake(SOME_ACTION)
it'll also be resolved immediately withEND
(see Semantics section for the why).I was planning on implementing this on the real-world example but due to lack of time i'll give a simpler example here
If we run the code i the client, then it'll be as with
while(true)
because noEND
action is dispatched on the client. On the server however, using for example React RouterAbove dispatching END will cause the while loops to break and the related saga to terminate its main body. With support for attached forks, a parent which has terminated its own body will wait for all forked children (attached by default) to terminate before returning. So the root saga will terminate after all fired tasks terminate.
There is on drawback though: we need to render twice: the 1st time to trigger the necessary actions and fire the load tasks, and the 2nd to send the final result to the client; but I dont think it's a big deal, because the time wasted on rendering would be non significant here compared to the latency of network request. And more importantly we can run the same code on the client and server.
Semantics of END
The motivation for the above behavior arises from the need to define precise semantics for
END
esp. how it should compose withinrace
andparallel
effects.For some time, I was confused because I looked to END from the Rx's point of view: i.e. as an end of a stream. But Actually there is no notion of stream in redux-saga, there is only notion of Futures (e.g. Promises): take(action), call(func), join(task) ... can all be viewed like normal function calls which return Futures (like await in async functions). So the issue become how do we translate
END
of a streams into the Future/Promise model.IMO the answer is the Never-happening-Future. For example suppose we have a kind of
nextEvent
method which returns the next event occurrence on a stream of events. What happens if we callnextEvent
on a stream that has already terminatedSince the stream is terminated, the promise should never resolve because there is no more future actions, so myFunc won't make any progress.
Once we define it this way, the sense of combining END with race and parallel becomes more obvious. We have a simple and precise algebra.
End
behaves like a sort of a Zero for Futures.This is how it's actually implemented in the proposal.
The doubt I'm having though is whether we should expose
END
explicitly to the developer or if we should handle it automatically by terminating the Saga. In the above example we used explicit handling of END. Now with automatic handling we can writeAbove there is no explicit END value; if the take resolves with an END. then we can choose to terminate the Saga automatically and resolve its return value with END. the END would then propagate to the parent, so it'll be also terminated. The only way to escape from END will be inside forked tasks and within race effects.
Automatic handling prevents us from dealing manually with END results. OTOH manual handling of END gives more flexibility (for example starting a second stream after a 1st one terminates)
The text was updated successfully, but these errors were encountered: