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

New Options #2

Open
6 tasks
okwolf opened this issue Aug 30, 2017 · 17 comments
Open
6 tasks

New Options #2

okwolf opened this issue Aug 30, 2017 · 17 comments

Comments

@okwolf
Copy link
Owner

okwolf commented Aug 30, 2017

Today the only available option is log to provide a custom logging function. It's a good escape hatch that's highly flexible, but I'm wondering if there should be other options in between. Not proposing to copy every option redux-logger supports, but maybe some are worth the effort.

Here are some proposed options:

  • filter - provide a filter function to decide which actions to log and which to ignore. The filter function is passed the {name, data} object describing each action. Defaults to show all actions by always returning true.
  • format - function to pre-process the state values before being passed to the logger. This allows for returning a new object with the state reorganized to be more readable or in a preferred String format. Defaults to the identity function.
  • diff - try to print an intelligent diff of what state the action updated. In the case of redux-logger deep-diff was used. Defaults to false.
  • performance - show how long the action took from start to end. Defaults to false.
  • timing - show the time the action began and/or ended. Defaults to false.
  • sameStateWarning - include a warning when not returning a new state object from an action. Defaults to false. See Logs the same object as prev and next state #3.
@jorgebucaran
Copy link
Collaborator

@okwolf Maybe list what options redux-logger provides (or any other logger) and then narrow the list down to only a few couple of must-haves.

@okwolf okwolf added the FEATURE label Aug 30, 2017
@okwolf
Copy link
Owner Author

okwolf commented Aug 30, 2017

@jbucaran good point. For now I'm going to keep drawing inspiration from redux-logger since I know it best, but I'm open to suggestions for other good loggers to borrow features from.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran see the updated list of proposed options above.

@jorgebucaran
Copy link
Collaborator

Excellent list @okwolf!

Other than sameStateWarning, these options sound good to me.

IMO filter, format and diff, in that order, should be the priority.

After that I'd consider timing (performance would be better since you'll be using the Performance API).

What do you think?

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran I agree with the priority, reordered the list. I also like the shorter name suggestions. To me performance is a better fit if the info displayed is how long each action took to complete but timing makes more sense if we're printing the full timestamp of the start/end to look through the log for which actions were clumped together in time.

@jorgebucaran
Copy link
Collaborator

timing makes more sense if you're printing the full timestamp of the start/end to look through the log for which actions were clumped together in time.

Gotcha. So, the question is which one is more useful or can we have both? ;)

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran Gotcha. So, the question is which one is more useful or can we have both? ;)

I think we should either have an option for only one of them, or both options. But not one option that does both.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran for filter do you think there's any value in providing the prevState and/or nextState for the filter function to decide logability? How about format for actions? 🤔

@jorgebucaran
Copy link
Collaborator

jorgebucaran commented Sep 11, 2017

I thought filter would just be an array with action names.

For format, I had forgotten we already have a custom log function, so now I think it's useless or rather, I'd prefer to use options.log than provide a cryptic format string.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran I thought filter would just be an array with action names.

That would be an alternate option, but my original proposal would take the full {name, data} object in case you wanted to decide based on different data for actions with the same name. Or if you had a pattern/prefix/suffix/RegExp to match for the names.

@jbucaran For format, I had forgotten we already have a custom log function, so now I think it's useless or rather, I'd prefer to use options.log than provide a cryptic format string.

Yeah if you provide log then you get full control over the formatting. This option would just be for overriding the formatting of state but leaving the rest of the look-and-feel to the log messages the default.

@jorgebucaran
Copy link
Collaborator

IMO sounds like it's trying to do too much. I would start with the minimal things that can be added to improve the logger just a bit. If a use case for another X or Y come up in the future, then definitely why not! :)

Alternatively, you could think for a way to add "plugins" to the logger, so that those solutions can exist outside @hyperapp/logger. Similar to how we push some things outside core in hyperapp.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran Alternatively, you could think for a way to add "plugins" to the logger, so that those solutions can exist outside @hyperapp/logger. Similar to how we push some things outside core in hyperapp.

One approach for extension would be to provide an option for plugins/mixins/middleware - each of which is a function that takes the parameters for the log function { prevState, action, nextState } as arguments and then returns the next one or false to cancel the log operation. I'm not sure if it would make sense to ever have more than one, but that would be a consistent API. This would supersede any need for implementing filter or format.

@jorgebucaran
Copy link
Collaborator

jorgebucaran commented Sep 11, 2017

This would supersede any need for implementing filter or format.

That would be pretty nice.

I think a way to do this could be to emit your own log event so that users, as well as mixins can tap into that event to provide filter / format functionality.

This then raises the issue of letting you define mixins in mixins again.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

Also timing and performance could be pushed outside the library by adding start and end properties to the action object to be used as desired.

Come to think of it, sameStateWarning could also be done inside a mixin-like thing or even using the existing log option.

Now I'm not sure which options should be priority anymore. 😕

@jorgebucaran
Copy link
Collaborator

Timing action duration could be up to a different mixin too.

@okwolf
Copy link
Owner Author

okwolf commented Sep 11, 2017

@jbucaran Timing action duration could be up to a different mixin too.

True, it could. But it might also make sense to group the performance information with the rest of the action info and state values in the log.

That code looks a bit familiar 😉 And your ignore reminds me of what you originally thought the filter option was for.

@naugtur
Copy link

naugtur commented Sep 11, 2017

As for same state warning, if it's there it should be in by default and point to a page with explanations why it's bad and what should be done about it.

My reasoning is, if you know what the flag is for, you don't need it much anymore.

@okwolf okwolf mentioned this issue Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants