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

Printing String objects as strings instead of objects #1450

Merged
merged 1 commit into from
Dec 26, 2017

Conversation

joeldenning
Copy link
Contributor

@joeldenning joeldenning commented Dec 22, 2017

This pull request updates the way that nodes are printed when being debugged.

Use case in question

// Sorta weird to provide a String object as a prop instead of a string, but React supports it.
<div className={new String('foo')} />

Previous behavior

Debugging the above node previously logged the following:

<div className={{...}} />

Proposed behavior

I propose it would make more sense to print props that are String objects as strings:

<div className="foo" />

Notes

Providing String objects for element props like className is something that is supported by the DOM, and thus supported by React.

  • See this react fiddle for an example of String objects being accepted as the className prop in React.
  • Run the following code in a browser console to see that this behavior is something supported by browsers themselves:
document.body.className = new String('foo');
document.body.className; // this will log "foo"

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.

It's absurd to ever use boxed primitives in JavaScript; but that said, enzyme should certainly handle it well even if people do bad things.

The console has superpowers; if we want to detect boxed strings, we have to have special-cased code for it.

However, another alternative is to use the object-inspect library for the "object" case, which already handles all boxed primitives, and might allow the Debug code to be greatly simplified.

@@ -32,7 +32,11 @@ function propString(prop) {
case 'boolean':
return `{${prop}}`;
case 'object':
return '{{...}}';
if (prop instanceof String) {
Copy link
Member

Choose a reason for hiding this comment

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

instanceof is never reliable, and should not be used.

If we want to detect boxed strings, we need to use the is-string library.

@@ -32,7 +32,11 @@ function propString(prop) {
case 'boolean':
return `{${prop}}`;
case 'object':
return '{{...}}';
if (prop instanceof String) {
return `"${prop.toString()}"`;
Copy link
Member

@ljharb ljharb Dec 23, 2017

Choose a reason for hiding this comment

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

String(x) is always strictly better than x.toString(), and inside a template literal, both are unnecessary for a non-Symbol - this can just be `"${prop}"`

@joeldenning
Copy link
Contributor Author

@ljharb thanks for the feedback - just pushed some 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 one nit, and a rebase

return '{{...}}';
if (isString(prop)) {
return `"${prop}"`;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

The else here isn’t necessary; the following return can be de-indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated

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.

2 participants