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

Proposal: createLogger can optionally log actions #987

Merged
merged 2 commits into from
Apr 23, 2020

Conversation

deini
Copy link
Contributor

@deini deini commented Oct 14, 2017

Proposal

Allow createLogger to optionally log actions, added a couple of options

{
  actionFilter,
  actionTransformer,
  logActions,
  logMutations
}

With some sane default to keep current functionality by default, this would be an opt-in option. Enabled by default as requested.

When enabled you get something like:

It just logs the action type and payload, since we cant tie actions to mutations, which I think its ok.

Note: Also added the devtoolHook for actions so we can hook it up later with devtools.

PS: I'm about to do a small plugin in case this proposal doesn't get approved and will update this PR with the url just in case someone is looking for this functionality. It would be awesome if we could have it built-in tho.

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks for improving the logger plugin 👍
I just commented about a few things.

logger.groupEnd()
} catch (e) {
logger.log('—— log end ——')
if (actionFilter(action, currentState)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't need to deep copy the state here since not retain prevState for actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call 💯

@@ -7,49 +7,83 @@ export default function createLogger ({
filter = (mutation, stateBefore, stateAfter) => true,
transformer = state => state,
mutationTransformer = mut => mut,
actionFilter = (action, state) => true,
actionTransformer = act => act,
logActions = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's ok to also enable logActions in default?

@deini
Copy link
Contributor Author

deini commented Oct 14, 2017

@ktsn Thanks for the review, addressed your comments, let me know if you want me to squash my commits.

@jeremyjh
Copy link

What is the status of this, will it be merged?

@GufNZ
Copy link

GufNZ commented Jan 11, 2018

Dangit - should have looked here first - I just wrote the same thing :-p

@ktsn ktsn added the proposal label Jan 11, 2018
@jandob
Copy link

jandob commented Oct 23, 2019

Almost 2 years later:
Again, will it be merged?

@kiaking
Copy link
Member

kiaking commented Apr 21, 2020

Sorry for the late response on this one, though it might be nice idea to have this...? It's pretty old so it might not be possible but @deini do you think you can rebase this one to the latest dev branch?

@deini
Copy link
Contributor Author

deini commented Apr 21, 2020

@kiaking rebased

@kiaking
Copy link
Member

kiaking commented Apr 23, 2020

Thanks a lot! Checked locally as well and it's working 👍

@kiaking kiaking merged commit 18be128 into vuejs:dev Apr 23, 2020
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.

6 participants