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

Debug utility #233

Merged
merged 7 commits into from
May 14, 2016
Merged

Debug utility #233

merged 7 commits into from
May 14, 2016

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented May 9, 2016

This PR addresses the TODO in #206 to add a logging utility. It is only used in the Dropdown for now as the Dropdown is our proving grounds for removing jQuery and setting our v1.0.0 precedence.

Two debug logs were generating object differences, see componentWillReceiveProps and componentDidUpdate. This logic was pulled into an objectDiff util and tested.

@levithomason levithomason force-pushed the feature/debug-util branch 4 times, most recently from 0286076 to b049602 Compare May 9, 2016 20:15
@@ -16,6 +18,8 @@ import DropdownItem from './DropdownItem'
import DropdownMenu from './DropdownMenu'
import Icon from '../../elements/Icon/Icon'

const debug = makeDebugger('dropdown')
Copy link
Member Author

Choose a reason for hiding this comment

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

Logs are auto namespaced to stardust:<your-name>

@levithomason levithomason force-pushed the feature/debug-util branch from b049602 to 5c14e4b Compare May 9, 2016 20:25
// }, {}))
debug.groupCollapsed('componentDidUpdate()')
debug('changed props:', objectDiff(nextProps, this.props))
debug.groupEnd()
Copy link
Member Author

Choose a reason for hiding this comment

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

console methods group, groupCollapsed, and groupEnd are added for cleaner console activity.

@dvdzkwsk
Copy link
Member

dvdzkwsk commented May 10, 2016

Couple of thoughts:

  1. Even though the debugger will noop during production, the arguments are still always evaluated. This is needlessly expensive in production or even outside of debug mode, especially for more complex operations. Consider lazily running debug statements, i.e.:
debug(() => object.diff(...))

The above is a solution if the following is not an option

  1. As a corollary, all of these debug statements are still invoked in production, their argument evaluation notwithstanding, even if they are just noop. I know it's a lot of work to wrap everything in environment checks, but for a public library I think it could be optimized a bit further. The pattern used here is similar to what we do in our business apps, but that's a bit of a different realm than a public library. Not sure if there's an easy way to abstract this logic so it's removed at compile time, but it should be considered.

@levithomason
Copy link
Member Author

Agreed on all counts and considered these things prior to the PR. In the future we'll have an optimization phase which will include a transform to remove debugs, static propTypes, and static_meta from production code.

I want to get more components moved to the 1.0 pattern before optimizing further. This PR moves us from console statements to a debug pattern we can iterate on.

@levithomason levithomason merged commit d623f47 into master May 14, 2016
@levithomason levithomason deleted the feature/debug-util branch May 14, 2016 17:26
@levithomason levithomason mentioned this pull request May 24, 2016
24 tasks
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