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

Lets wrap reducer #276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

srghma
Copy link

@srghma srghma commented Dec 5, 2017

@morgs32
Copy link

morgs32 commented Dec 9, 2017

@evgenyrodionov If this will remedy the order issue please let's move this along!

@tswaters
Copy link

tswaters commented Jan 3, 2018

This requires a change of the API -- and not using middleware at all!

Before:

createStore(
  combineReducers({....}),
  initialState,
  applyMiddleware(...[thunk, etc, logger])
)

After

createStore(
  logger(combineReducers({....})),
  initialState,
  applyMiddleware(...[thunk, etc])
)

Actually, I'm not sure how that would work with combineReducers - the tests only show one reducer. Hopefully that works. FWIW, I'm not a huge fan of this approach.

It would make way more sense to start the group, log action, before... call into next, and log once more with the result, errors & diff & close the group. This has the side effect (feature?) that each stray console.log message would go inside the group for a given action... not much that can be done about that.

Unfortunately, it's a bit of a job to do this given the way this module is written currently with printBuffer being tightly coupled with the logEntry object, and printing being done all at once when finished.

@srghma
Copy link
Author

srghma commented Jan 3, 2018

@tswaters I don't think this will work, as long as I remember the the problem was how redux middleware works.

Do you have example/test, that can show wrong ordering?

@tswaters
Copy link

tswaters commented Jan 3, 2018

the problem was how redux middleware works.

Middleware functions just change dispatch. It just so happens to be a really good injection point for logging. Calling next on the last piece of middleware is calling dispatch, which effectively fires the reducer(s).

The problem is -- as I understand it -- the values for before/after/error/etc are all stored and everything is logged after the reducer fires. i.e., "before" and "action" are logged after the reducer does it's thing. This means if you log anything in, say, componentWillReceiveProps (which will be invoked if the reducer changes anything), it will appear prior to the group for a given action.

This is how I'd fix it - and indeed this is basically the "logging" example from the redux docs.

store => next => action => {
  let returnValue = null
  let error = null
  console.group(action.type)
  console.log('before', store.getState())
  console.log('action', action)
  try {
    returnValue = next(action)
  } catch (err) {
    error = err
  }
  console.log('after', store.getState())
  if (error) { console.log('error', error) }
  console.groupEnd()
  if (error) { throw error }
  return returnValue
}

Of course, there are a lot of transformations and extra things this library does, meaning my code above isn't as easy as it appears --- all the logging stuff is in ./core and the interactions with redux is in ./index. There would need to be injection points in ./core at the various stages (started/before/action before calling next and after/error/diff/took afterwards).

The group would need to change too - you can't know how long the action took if it hasn't happened yet! This is a bit of a change of the API so technically its a semver major. Not sure how many people take advantage of titleFormatter but it could no longer receive the took parameter.

@MichelSimonot
Copy link

This has the side effect (feature?) that each stray console.log message would go inside the group for a given action... not much that can be done about that.

What about splitting the log into two parts?

store => next => action => {
  let returnValue = null
  let error = null

  // Log the "start" of the action sequence.
  console.group('action dispatch', action.type)
  console.log('before', store.getState())
  console.log('action', action)
  console.groupEnd()

  try {
    returnValue = next(action)
  } catch (err) {
    error = err
  }

  // Log the "result" of the action sequence.
  console.group('action results', action.type)
  console.log('action', action)
  console.log('after', store.getState())
  if (error) { console.log('error', error) }
  if (diff) { do diff stuff }
  console.groupEnd()

  if (error) { throw error }
  return returnValue
}

Or grouping the logs triggered by the action into the action log?

store => next => action => {
  let returnValue = null
  let error = null
  console.group(action.type)
  console.log('before', store.getState())
  console.log('action', action)

  // Add a second layer of grouping for anything
  //    logged because of the action.
  console.group()
  try {
    returnValue = next(action)
  } catch (err) {
    error = err
  }
  console.groupEnd()

  console.log('after', store.getState())
  if (error) { console.log('error', error) }
  console.groupEnd()
  if (error) { throw error }
  return returnValue
}

Just throwing ideas out there. I think the first would be better, but neither are really ideal.

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

Successfully merging this pull request may close these issues.

5 participants