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.

Sign up for GitHub

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

Jump to bottom

Implement verbose debug output #1547

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Implement verbose debug output #1547

merged 1 commit into from
Mar 14, 2018

Conversation

blainekasten
Copy link
Contributor

This implements .debug({verbose: true}) which prevents boxing primitive values when stringified.

Fix for #1476

@blainekasten blainekasten requested a review from ljharb March 1, 2018 15:47
ljharb
ljharb requested changes Mar 1, 2018
View reviewed changes
packages/enzyme-test-suite/package.json Outdated
@@ -49,4 +49,4 @@
"eslint-plugin-jsx-a11y": "^6.0.3",
"eslint-plugin-react": "^7.6.1"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure all files always have a trailing newline :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, not sure how this one happened. I'll fix it.

packages/enzyme-test-suite/test/Debug-spec.jsx Outdated
));
});

it('options.verbose causes boxed primitives to be unboxed', () => {
Copy link
Member

Choose a reason for hiding this comment

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

None of these are boxed primitives; a boxed primitive is like Object(2). Do you mean objects and arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. tbh, i heard you say it once and I didn't really know what it meant, so like everyone, I just pretended it was the right thing. my bad. I'll fix up the wording to just mean objects and arrays.

packages/enzyme/src/Debug.js Outdated
@@ -43,15 +44,19 @@ function propString(prop) {
return `{${inspect(prop)}}`;
}
if (typeof prop === 'object') {
if (options.verbose) {
return `{${CircularJSON.stringify(prop)}}`;
Copy link
Member

Choose a reason for hiding this comment

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

Why not inspect(prop)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me test this. I assumed it wouldn't do what I wanted it to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently the difference between the two libs:

{ 
  inspect:      <div data-json={{ a: [ 1, 3, { true: true } ], b: false, c: { d: 'f' }, d: [ 1, 3, { true: true } ] }} data-arry={[ 1, 2, { f: { d: 'f' } } ]}>
  circularJSON: <div data-json={{"a":[1,3,{"true":true}],"b":false,"c":{"d":"f"},"d":"~a"}} data-arry={[1,2,{"f":{"d":"f"}}]}>
}

Seems like the inspect one looks more like real code, rather than json. But It will definitely crash if used against circular structures since it just prints them out directly. If I pass in something like:

a = {};
a.b = {};
a.c = a.b;
a.b.c = a.c;

Passing that object as a prop will then crash the method.

We are using this method in enzyme-matchers and I think it would be really confusing if anyone ever ran into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually. Just tested and it looks like inspect does stringify circular properly! So i'll use that.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome :-) I'm not particularly worried about circular cases anyways, but glad it handles it!

@blainekasten blainekasten force-pushed the task/verbose-debug branch 3 times, most recently from 5955791 to 59dc2d0 Compare March 1, 2018 19:17
@blainekasten
Copy link
Contributor Author

@ljharb addressed your concerns dawg

ljharb
ljharb reviewed Mar 1, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending some docs changes

packages/enzyme-test-suite/test/Debug-spec.jsx Outdated
@@ -684,5 +708,31 @@ describe('debug', () => {
</Bar>`
));
});

it('options.verbose causes boxed primitives to be unboxed', () => {
Copy link
Member

Choose a reason for hiding this comment

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

let's update this description also

packages/enzyme/src/ReactWrapper.js Outdated
* options.ignoreProps - if true, props are omitted from the string.
* @param {Object} [options] - Property bag of additional options.
* @param {boolean} [options.ignoreProps] - if true, props are omitted from the string.
* @param {boolean} [options.verbose] - if true, boxed primitives are unboxed.
Copy link
Member

Choose a reason for hiding this comment

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

also here

packages/enzyme/src/ShallowWrapper.js Outdated
* options.ignoreProps - if true, props are omitted from the string.
* @param {Object} [options] - Property bag of additional options.
* @param {boolean} [options.ignoreProps] - if true, props are omitted from the string.
* @param {boolean} [options.verbose] - if true, boxed primitives are unboxed.
Copy link
Member

Choose a reason for hiding this comment

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

also here

@nfcampos
Copy link
Collaborator

nfcampos commented Mar 1, 2018

Should we add a test for verbose: false that has arrays or objects to test the difference in behaviour between verbose false and true?

LGTM otherwise

@ljharb
Copy link
Member

ljharb commented Mar 1, 2018

Yes, that's a great idea

@blainekasten
Copy link
Contributor Author

@nfcampos thanks for the call out. I added that and addressed the rest of @ljharb's comments

@blainekasten blainekasten force-pushed the task/verbose-debug branch 2 times, most recently from c3925de to c9761f9 Compare March 2, 2018 17:22
@blainekasten
Copy link
Contributor Author

ping @ljharb

@ljharb
Copy link
Member

ljharb commented Mar 13, 2018

@blainekasten I don't see any tests for verbose: false?

@blainekasten
Copy link
Contributor Author

@ljharb https://github.com/airbnb/enzyme/pull/1547/files#diff-3cc78530daa735c5d407c3f768d35c57R743

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

aha, thanks :-) i'll rebase it and rereview.

ljharb
ljharb approved these changes Mar 14, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb merged commit 0fe0d73 into enzymejs:master Mar 14, 2018
@ljharb ljharb mentioned this pull request Mar 14, 2018
Closed
@blainekasten blainekasten deleted the task/verbose-debug branch April 12, 2018 12:16
This was referenced Aug 8, 2018
Open
Merged
Closed
Closed
This was referenced Oct 1, 2018
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@nfcampos nfcampos Awaiting requested review from nfcampos

@lelandrichardson lelandrichardson Awaiting requested review from lelandrichardson

@aweary aweary Awaiting requested review from aweary

Assignees
No one assigned
Labels
API: mount API: shallow semver: minor New stuff.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@blainekasten @nfcampos @ljharb