Use toString() on object interpolation when defined

[mathieutu]

Version

2.5.16

Reproduction link

Steps to reproduce

When interpoling a object, it uses the JSON.stringify method instead of toString, which is normally the proper method to set a way to convert an object to a string. We could try to see if the toString method is present before calling the toJSON one ?

What is expected?

In the fiddle:

string: test

What is actually happening?

In the fiddle:

string: { "json": "jsonVal" }

---

Thanks.

[Gaya]

Just stumbled upon this myself found the culprit here: https://github.com/vuejs/vue/blob/dev/src/shared/util.js#L81

Would be nice if it can execute the object's toString, but maybe there is a reason for doing it like this?

[01045972746]

I think toJSON is better because all of component's options (structure) are JSON.
And.... If it was toString, there should be changed to json again maybe.

[Gaya]

In this case we’re representing an object as a string, which is what toString is made for. I believe JSON.stringify is only used to make prettier outputs or Object and Array when they have no nice toString formatting.

We might get around this by checking if the object’s toString method is the last in the prototype chain. This way we can use toString if we added it to the prototype ourselves

[01045972746]

export function toString (val: any): string {
  return val == null
    ? ''
    : typeof val !== 'object'
      ? String(val)
        : val.toString() === _toString.call({})
          ? val.toString()
            : JSON.stringify(val, null, 2)
}

Is it right? It is just checking with Object's toString and parameter Object's toString.
I have edited this in that https://github.com/vuejs/vue/blob/dev/src/shared/util.js#L81 file.

[mathieutu]

@01045972746 With my comprehension of this, it looks good, thanks!
Do you want to submit a PR, or I do it?

[01045972746]

@mathieutu Umm.. i have tested it, but it is failed because of [].

Like this,

SUMMARY:
✔ 1096 tests completed
✖ 2 tests failed

FAILED TESTS:
  Directive v-html
    ✖ should support all value types
      PhantomJS 2.1.1 (Linux 0.0.0)
    Expected '' to be '[]'.
    webpack:///test/unit/features/directives/html.spec.js:43:36 <- index.js:169041:36
    shift@webpack:///test/helpers/wait-for-update.js:24:32 <- index.js:102129:36
    webpack:///src/core/util/next-tick.js:95:16 <- index.js:75156:16
    flushCallbacks@webpack:///src/core/util/next-tick.js:16:4 <- index.js:75048:14

  Directive v-text
    ✖ should support all value types
      PhantomJS 2.1.1 (Linux 0.0.0)
    Expected '' to be '[]'.
    webpack:///test/unit/features/directives/text.spec.js:29:36 <- index.js:173023:36
    shift@webpack:///test/helpers/wait-for-update.js:24:32 <- index.js:102129:36
    webpack:///src/core/util/next-tick.js:95:16 <- index.js:75156:16
    flushCallbacks@webpack:///src/core/util/next-tick.js:16:4 <- index.js:75048:14

We have to make it more elaborate!

[Gaya]

I’ll take a look when I get the time. Most likely next monday

[mathieutu]

Hey guys, I've took time to work on it this night.
Here it is: #8217
Thanks for your help!

[Gaya]

@mathieutu looks good, but what about using something like:

export function toString (val: any): string {
  return val == null
    ? ''
    : typeof val === 'object' && Object.getPrototypeOf(val).toString === val.toString
      ? JSON.stringify(val, null, 2)
      : String(val)
}

It checks if the toString method has been overwritten without comparing values. I think this would be better for performance.

[mathieutu]

Actually I'm kind of already doing this. I don't compare values but function references :

val.toString === _toString

_toString is defined here: https://github.com/mathieutu/vue/blob/d2f10269ee274703c43051d6ee98c2177f0a2ec0/src/shared/util.js#L48

The only difference is that I check before if the value is a plain object. This is the only place where a value is used, but I can't say anything in term of performance about it.

[Gaya]

@mathieutu ah, didn't know it was used that way, awesome. Then maybe the isPlainObject check is not needed? Or maybe move it as the last check? We could do some performance testing for it.

[mathieutu]

Ok, I've tested your solution with a more complicated test, and it doesn't work.

If you test with a value like Object.create({ toString () { return 'foo' } }) the result will not be the one we want.

The isPlainObject is needed to avoid arrays. But in fact we could make the same comparison for Arrays. I've tried to do some benchmark tests on it, but I don't have anything really relevant between both ways.

https://repl.it/repls/RudeCrazyQuerylanguage

[Gaya]

@mathieutu If I use the Object.getPrototypeOf(val).toString === val.toString check on the object returned by Object.create({ toString () { return 'foo' } }) it works just fine. This check also seems to work for Arrays. Is Object.prototype.toString the correct way of checking in this case?

[Gaya]

@mathieutu Hmmm, same happens if I compare to Object.prototype.toString. Why are we trying to avoid Arrays? Seems like they work just the same as objects in their toString method

[mathieutu]

@Gaya I don't have the time right now to explain everything, so I let you play with the unit tests (test with arrays avoiding, and not).

And you can't really count on getPrototypeOf(val) because we don't know what is the first prototype of the val. (Check the doc here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getPrototypeOf).

[elmatou]

I don't know where we stand on this request (which I support), but JS make an extensive use of coercion, and when needing a string it call for toString() as in String(object).

I think this is the proper way to go, just ask for a coercion with String(object) while rendering

[mathieutu]

@elmatou This was merged in core last year, so it should work pretty well now!