Skip to content
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

Return value of dispatch #61

Closed
gaearon opened this issue Jun 9, 2015 · 20 comments
Closed

Return value of dispatch #61

gaearon opened this issue Jun 9, 2015 · 20 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Jun 9, 2015

@vslinko

What should redux.dispatch() return? Currently the code is

    function dispatch(action) {
      return typeof action === 'function' ?
        action(dispatch, state) :
        dispatchSync(action);
    }

but I don't remember the circumstances when the return value of the callback form can be useful.

Because the callback form may schedule additional dispatches, it seems that the calling code still doesn't have enough information. Can we just return whatever action was passed?

    function dispatch(action) {
      if (typeof action === 'function') {
        action(dispatch, state);
      } else {
        dispatchSync(action);
      }
      return action;
    }

?

@vslinko
Copy link
Contributor

vslinko commented Jun 9, 2015

My usage example

export function runTransition(url) {
  return async perform => {
    const route = routing
      .filter(({route}) => route === url)
      .shift() || notFound

    const transition = transitions[route.transition]
    const componentKey = route.component

    const {title, status, ...props} = await perform(transition())

    perform(changePage(componentKey, props))

    return {
      url, title, status, componentKey, props
    }
  }
}

export function navigateTo(url) {
  return async perform => {
    progressIndicator(async () => {
      const {title} = await perform(runTransition(url))

      document.title = title
      history.pushState({url}, title, url)
    })
  }
}

I have runTransition action creator that fetching all data. This AC called on server side.
And I have navigateTo AC that calling runTransition and touching history API. This AC called on client side.

@vslinko
Copy link
Contributor

vslinko commented Jun 9, 2015

Server side usage example:

export default async function renderPage(url) {
  const dispatcher = createDispatcher()

  const {title, status} = await dispatcher.perform(runTransition(url))

  const atom = dispatcher.getAtom()

  const pageContent = React.renderToString(
    <Application dispatcher={dispatcher} />
  )

  return {
    status,
    body: appTemplate(title, pageContent, atom)
  }
}

@gaearon
Copy link
Contributor Author

gaearon commented Jun 9, 2015

Thank you for the examples! Let's leave this open for now, I'll come back to it later..

@acdlite
Copy link
Collaborator

acdlite commented Jun 9, 2015

Returning a promise is the uber example. In all the Flux code I've ever written, I've only ever needed the return value for that reason.

@leoasis
Copy link
Contributor

leoasis commented Jun 9, 2015

I'm not sure if the dispatch should return anything at all, the whole idea around the dispatcher is to do "fire and forget" and handle data that follows that action as app state, whenever it is received.

@acdlite
Copy link
Collaborator

acdlite commented Jun 9, 2015

I'm in favor of the the base dispatch() method return nothing. For those cases where you do need it to return a promise, that is easily implemented at the middleware level (refer to #63):

function promiseMiddleware(next) {
  return action =>
    action && typeof action.then === 'function'
      ? action.then(next)
      : next(action);
}

The dispatcher is still fire and forget because it has no notion that a promise was ever involved. It merely receives a payload object and sends it through the store, synchronously. But the caller can still know the dispatcher has completed because the call to the dispatcher was proxied by the middleware.

In Flummox, the original behavior for async actions was to return a promise to the caller, but it would always resolve to undefined. That way they could know the operation was complete, but it would prevent users from breaking the Flux flow. Eventually I removed this behavior to make testing of action creators easier. This isn't a problem in Redux because action creators are pure functions.

Furthermore, in Redux, we can let the users decide if and how they want promise handling to work by making it easy to write their own custom middleware.

@emmenko
Copy link
Contributor

emmenko commented Jun 9, 2015

I also think the dispatcher should just "fire and forget"

@vslinko
Copy link
Contributor

vslinko commented Jun 9, 2015

But how to implement action creators from my example if dispatch doesn't return anything?

@leoasis
Copy link
Contributor

leoasis commented Jun 9, 2015

I don't know exactly what those transition() functions are doing and why you need to dispatch them. If runTransition is not being called as an AC, and only via navigateTo and renderPage then you can inject it the dispatch (or perform) method and make it a function that returns something and calls that dispatch as well. You have full control of that on your side, so no need to have the dispatcher return anything. Same thing can apply to those transition() functions

@acdlite
Copy link
Collaborator

acdlite commented Jun 9, 2015

@vslinko Couldn't you just get it from the atom? You have access to it right there a few lines down.

@acdlite
Copy link
Collaborator

acdlite commented Jun 9, 2015

Middleware can always return things, but the base dispatch method would not.

@vslinko
Copy link
Contributor

vslinko commented Jun 10, 2015

I don't know exactly what those transition() functions

Transition is action creator that fetches all data for page.
Transition should be action creator because it could change application state.
Transition should return value for runTransition
Simplified example:

return function homepageTransition() {
  return async perform => {
    const apiResponse = (await homepageApiRequest())

    perform(mergeResources(collectResources(apiResponse)))

    return {
      status,
      title,
      queryParams,
      apiResponse,
    }
  }
}

runTransition is another action creator that mathes transition by url.
runTransition should be action creator because it could change application state.
runTransition should return value for navigateTo
Actial example:

export function runTransition(url) {
  return async perform => {
    const route = routing
      .map(route => {
        return {...route, match: matchRoutePattern(route.route, url)}
      })
      .filter(({match}) => !!match)
      .shift()

    if (!route) {
      return perform(runNotFoundTransition(url))
    }

    const transition = transitions[route.transition]
    const componentKey = route.component

    const {title, status, ...props} = await perform(transition(route.match || {}))

    if (status === 404) {
      return perform(runNotFoundTransition(url))
    }

    if (status === 500) {
      return perform(runErrorTransition(url, props))
    }

    perform(changePage(componentKey, props))

    return {
      url, title, status
    }
  }
}

navigateTo is action creator too.
navigateTo should be action creator because it should perform another action creator.
Simplified example:

export function navigateTo(url) {
  return async perform => {
    const {title} = await perform(runTransition(url))

    document.title = title
    history.pushState({url}, title, url)
  }
}

@vslinko Couldn't you just get it from the atom? You have access to it right there a few lines down.

Only on server side. But how to get it in navigateTo?

@leoasis
Copy link
Contributor

leoasis commented Jun 10, 2015

I think you can solve your problem if you manually call the other actionCreators by manually passing the perform function, which you already have in your root actionCreator. Then you can do:

instead of this:

const {title} = await perform(runTransition(url))

You do this:

const {title} = await runTransition(url)(perform)

And the same inside the runTransition action creator:

Instead of this:

const {title, status, ...props} = await perform(transition(route.match || {}))

You do this:

const {title, status, ...props} = await transition(route.match || {})(perform)

So conceptually you only call the dispatcher with one action creator from outside, and that action creator dispatches several actions, only the logic is spread in other functions that receive the dispatch function.

Another analysis that should be done is if you really need the transitions and the runTransition functions to actually be action creators (that is, actually called from the outside app directly), or just functions that can have the dispatch function injected.

This is just an idea, let me know if it works at all or not

@vslinko
Copy link
Contributor

vslinko commented Jun 10, 2015

I think you can solve your problem if you manually call the other actionCreators by manually passing the perform function, which you already have in your root actionCreator. Then you can do:

Yes, I know that.
Moreover I'm created PR that adds return to dispatcher because I don't want to pass perform and state into action creator by myself.
My original concerns described here #48 (comment)

@leoasis
Copy link
Contributor

leoasis commented Jun 10, 2015

Yes, you're right about that, you don't want to pass state just because. Anyway, I still think you shouldn't need to use the returned values from functions. For example in the navigateTo AC, you need the title and the url to set the document.title and the historyApi. If you managed those values in your app state (i.e. handled by a Store), you can then just model those inside your React application as components at the root level like <DocumentTitle /> and <CurrentUrl />, that render nothing to the dom but modify something outside (using componentDidUpdate for example). Another option is to have an extra subscribe on the dispatcher and listen to state changes and directly set the title and url in that listener.

The server side part is trickier I think, since it's hard to know when there are no remaining actions to be dispatched if we cannot await on anything.

@acdlite
Copy link
Collaborator

acdlite commented Jun 10, 2015

You can await a promise if you use middleware. Just because the dispatcher doesn't return anything doesn't mean your middleware can't. See my example above #61 (comment)

@leoasis
Copy link
Contributor

leoasis commented Jun 10, 2015

Makes sense. But now I'm wondering if I didn't want to use promises but still wanted to know when the dispatch ends on the server side in order to send the request, how would I do that if i have ACs using the async function way?

@vslinko
Copy link
Contributor

vslinko commented Jun 10, 2015

@leoasis if you don't use promises and async/await you could subscribe to dispatcher and wait for store changes. Your AC could set flag completed in store. This approach looks ugly, so this is my reason to return value from AC.

@acdlite I don't mind about dispatcher "fire and forget" if middleware could change that behaviour. As I see middleware already in master so I'm satisfied.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 13, 2015

Is this solved my middleware in 0.10? Can we close?

@vslinko
Copy link
Contributor

vslinko commented Jun 13, 2015

I didn't updated to 0.10 yet, so I don't know.
I think you could close this issue. I can (re)open issue If I'll have some questions.

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

No branches or pull requests

5 participants