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

Implement verbose debug output #1547

Merged
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

@@ -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.

));
});

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.

@@ -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
Copy link
Contributor Author

@ljharb addressed your concerns dawg

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

@@ -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

* 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

* 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.

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
@blainekasten blainekasten deleted the task/verbose-debug branch April 12, 2018 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants