Skip to content

Add logger predicate #5

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

Merged
merged 1 commit into from
Aug 19, 2015

Conversation

vovacodes
Copy link
Contributor

Implementation for https://github.com/fcomb/redux-logger/issues/4.

const logger = createLogger({
  predicate: (getState, action) => action.type !== AUTH_REMOVE_TOKEN
});
const createStoreWithMiddleware = applyMiddleware(logger)(createStore);

@vovacodes
Copy link
Contributor Author

@imevro
Copy link
Collaborator

imevro commented Aug 15, 2015

Hey, thanks!
Why we need predicate? I think something about

const logger = applyLoggers(diffLogger(), simple('warn'));
applyMiddleware(logger)

If we can do this, it's will be very handy: we can use diff-logger or custom debugger which save to localStorage (for example) as extension. And, of course, ignored actions (#4) too.

@vovacodes
Copy link
Contributor Author

Let's split the answer into two parts.

Why do I think that passing options object into logger creator function is better idea than to have special parameters like in your example: applyLoggers(diffLogger(), simple('warn'))

That is because the module's API wouldn't need to be changed in a breaking manner each time we decide that we need some other configuration option:

// API is createLogger(options)
const logger = createLogger({
  predicate: ...,
  collapsed: true
});

// then we decided to add "logFunc" option to specify custom log function instead of console.log
// API is still the same - createLogger(options)
const logger = createLogger({
  predicate: ...,
  collapsed: true,
  logFunc: ...
});

// VS

// API is createLogger(logFunc, logLevel)
const logger = createLogger(diffLogger(), simple('warn'));

// We decided to add "collapsed" parameter to switch to use console.groupCollapsed
// API changed - createLogger(logFunc, logLevel, collapsed);
// We have to bump the package's minor or even major version
const logger = createLogger(diffLogger(), simple('warn'), true);

Another question why to use predicate instead of say logLevel
Because in this case user could define her own rules of when to log or when to bypass.

Say we want to disable logs in production:

const logger = createLogger({
  predicate: () => ! conf.production
});

To log only important actions

const logger = createLogger({
  predicate: (getState, action) => action.meta.important
});

And so on. It is way more flexible for consumer of the module and nicely fits in with functional paradigm

@imevro
Copy link
Collaborator

imevro commented Aug 15, 2015

Not exactly. In my idea createLogger is function which iterates over arguments and return all loggers.

So,

function createLogger(...loggers) {
  loggers.forEach(logger => );
}

function simple(collapsed, level) {
  collapsed ? console.group() : console.groupCollapsed();

  console[level](message);

  console.groupEnd();
}

createLogger(diff(), simple(true, 'warn'));

So we can release redux-logger as core library, but redux-logger-diff, redux-logger-simple, redux-logger-http and etcetera as separate modules.

But I have one question: do we really need all of these? It looks like meta-middleware for redux's middleware. Why we can't release separate packages and pass them in applyMiddleware?

Don't think I want to reject your PR, of course not. But I want build better API for long-term and future extensions and we need investigate do we really need it or it's overengineering.

@vovacodes
Copy link
Contributor Author

But I have one question: do we really need all of these? It looks like meta-middleware for redux's middleware. Why we can't release separate packages and pass them in applyMiddleware?

I agree here, we definitely don't want to do the applyMiddleware's job:

applyMiddleware(diffLogger, simpleLogger, etc);

But we still have to provide a mechanism of bypassing logging for redux-logger, because it is essential for every logger library. Do you agree, @theaqua?

@imevro
Copy link
Collaborator

imevro commented Aug 16, 2015

@wizardzloy yep. I think we don't need invent another applyMiddleware for that small functions, but we need options. I will release 1.0 (#1 #3 #4) based on your PR today or tomorrow. Thanks for your help!

@vovacodes
Copy link
Contributor Author

@theaqua thanks for the productive discussion

imevro pushed a commit that referenced this pull request Aug 19, 2015
@imevro imevro merged commit bf3fefa into LogRocket:master Aug 19, 2015
@vovacodes vovacodes deleted the feature/add-logger-predicate branch August 20, 2015 10:30
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.

2 participants