Change prop type checking to use instanceof instead of typeof

[DanielCoulbourne]

What problem does this feature solve?

Currently prop type checking fails when passing in an object that extends the expected type.
This would allow anything inheriting from the expected type to pass the check.

What does the proposed API look like?

vue/src/core/util/props.js

Line 148 in 8d66691

valid = typeof value === expectedType.toLowerCase()

If this line were

valid = value instanceof expectedType.toLowerCase()

Then child classes would also pass. In my mind if a component is checking for String and I pass in an object that extends string this should pass the typecheck.

This was born out of an issue that one of my users has passing an String castable object which extends string as a prop into a Vue component.

If you're interested you can check that out here:

tighten/ziggy#64

[jkzing]

Actually this could already be done with custom prop validator, in your case:

Vue.component('custom', {
  props: {
    foo: {
      default: '',
      validator: value => value instanceof String
    }
  }
})

[DanielCoulbourne]

@jkzing this would absolutely work for the use-case where I am creating a component. The use case I'm interested in, however is when using components from vendor code. If a vendor component checks for String type there is nothing I can do but manually call toString on my object, which is a little janky.

There are only two good reasons for a vendor package to check for a type:

1. They want to use methods on that type's prototype, like String().replace() (all parent prototype methods are available on wrapper objects so this shouldn't be an issue)
2. They want to use the argument as that type (printing a string, iterating an array, etc.)

Neither of these use-cases is broken by allowing users to pass in wrapper objects, so there is no inherent value to strictly checking for type literals. Checking that something is strictly a String gains you nothing over checking that something has all the properties of a String.

I think this is akin to failing a test for Animal when a user provides Dog which extends Animal

[HerringtonDarkholme]

I'd rather argue mixing primitive values and their wrapper objects is code smell in JavaScript.

e.g. https://eslint.org/docs/rules/no-new-wrappers

Supporting this checking in API means Vue, implicitly or indirectly, encourages code smell.

Consider, if vendor code returns two wrapper objects with the same underlying primitive value, Vue will treat two objects as different, but that's not the case of primitive value. Users of such libraries should explicitly convert wrapper to primitive value (by cast or toString).

[DanielCoulbourne]

Could you give an example of a situation where your example would apply? I'm having a hard time understanding. There is no situation where you could pass two wrapper objects into a single prop, so I don't see the issue.

As far as this being a "code smell", I believe that code smell is an indication that something MIGHT be wrong, not an guarantee that something IS wrong.

I don't buy the argument that by allowing people to use a language feature like wrappers, Vue is somehow endorsing or encouraging it, and I also don't buy that wrappers are a moral evil on which a framework like Vue must take a stand.

This is a language feature and there are good and bad reasons to use it. If you decide to design a framework to prevent people from using wrappers badly, you also prevent people from using them well. I think programmers can be trusted.

[HerringtonDarkholme]

Consider:

<my-comp :prop="Route(route)"/>

If route is updated to same value, say this.route = oldRoute. The above code will trigger my-comp re-render because Route is an object. But it should not because route value is the same.

Also, if String allows wrapper object, how users can check if prop is primitive? Using validator. But you can use validator in current api, too.

[DanielCoulbourne]

I understand what you're saying and that would not be good. To me it seems like:

On the one hand –
By switching to instanceOf you run the risk of a developer writing bad code which triggers unnecessary renders. It seems to me that passing in an object which needlessly resets its value unnecessarily would ALWAYS cause a re-render, not just on a String typed prop, so the onus is on the developer of the Route object (in this case, me) to deal with those issues.

On the other hand –
You prevent developers from using wrappers in their code, even if that is the best solution. This doesn't just apply to my code, but also to large commonly used libraries like Lodash, which uses wrappers for its commonly used _.chain() method.

It is obvious that in this case I (partly selfishly, but also out of principle) prefer the scenario that gives developers the freedom to use wrappers (and more generally: whatever language feature fits their problem best) but also saddles them with the responsibility of dealing with the fallout in the edge-cases.

"With great power comes great responsibility" and all that 😄

Like I said above, I think that developers can handle the responsibility, and I think that adding this feature is the right move. But I also understand that this is open source and my opinion is not the only one that matters. I leave it up to the community to decide.

Thanks for a fun discussion 😄 👍