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

Warn against duplicate props/data fields #1166

Closed
yyx990803 opened this Issue Aug 17, 2015 · 21 comments

Comments

Projects
None yet
@yyx990803
Member

yyx990803 commented Aug 17, 2015

Problem

Currently the props are merged with the instance's own data, which has a few problems:

  1. Both data and props can declare the same property name. The order and priority of the merging is not clear. Should a prop overwrite the field with the same name in data? Or the other way around? Currently, the props' initial values are available inside data functions, but the same field returned in data gets overwritten by the prop.
  2. Once merged, it's not clear whether a component property is its "private" state or a prop that is passed down from (and thus can be changed by) the parent, unless you go and check the component's props declaration.

Proposal

Move props into its own name space, for example this.props. This avoids the merging problem (we can either make props a reserved field or make it $props) and makes it explicit that something is a prop that is obtained from outside the component.

Throw a warning if the user defines duplicate fields in data and props.

@yyx990803 yyx990803 changed the title from Put props into its own namespace? to Put props into its own namespace Aug 17, 2015

@yyx990803 yyx990803 modified the milestone: 1.0.0-alpha Aug 17, 2015

@kazupon

This comment has been minimized.

Show comment
Hide comment
@kazupon

kazupon Aug 17, 2015

Member

👍
That sounds great !!
I hope especially to improve props namespace issue.

Member

kazupon commented Aug 17, 2015

👍
That sounds great !!
I hope especially to improve props namespace issue.

@dgerber

This comment has been minimized.

Show comment
Hide comment
@dgerber

dgerber Aug 17, 2015

Contributor

Calling it $props would make clear that it's reserved.

Contributor

dgerber commented Aug 17, 2015

Calling it $props would make clear that it's reserved.

@Mat-Moo

This comment has been minimized.

Show comment
Hide comment
@Mat-Moo

Mat-Moo Aug 17, 2015

Contributor

How will this work with 2 way binding? At present I send a prop (which can be blank) and my data defines defaults and this works nicely. By separating them I would need to merge defaults with the prop data? Or create the correct data structure before creating the component (But I'm trying to make the component responsible for the data not the app). Make sense?

Contributor

Mat-Moo commented Aug 17, 2015

How will this work with 2 way binding? At present I send a prop (which can be blank) and my data defines defaults and this works nicely. By separating them I would need to merge defaults with the prop data? Or create the correct data structure before creating the component (But I'm trying to make the component responsible for the data not the app). Make sense?

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 Aug 17, 2015

Member

@Mat-Moo not sure if I get what you mean. You can declare defaults for props directly:

props: {
  a: {
    type: Number,
    default: 123
  }
}
Member

yyx990803 commented Aug 17, 2015

@Mat-Moo not sure if I get what you mean. You can declare defaults for props directly:

props: {
  a: {
    type: Number,
    default: 123
  }
}
@Mat-Moo

This comment has been minimized.

Show comment
Hide comment
@Mat-Moo

Mat-Moo Aug 17, 2015

Contributor

Hmm I'm using v-repeat on an array to create components so data comes in anyway so maybe irrelevant.

Contributor

Mat-Moo commented Aug 17, 2015

Hmm I'm using v-repeat on an array to create components so data comes in anyway so maybe irrelevant.

@mark-hahn

This comment has been minimized.

Show comment
Hide comment
@mark-hahn

mark-hahn Aug 17, 2015

In functions, vars passed via parameters and local variables are in the
same space. Vue should stay as it is.

On Sun, Aug 16, 2015 at 10:04 PM, Evan You notifications@github.com wrote:

Problem

Currently the props are merged with the instance's own data, which has a
few problems:

Both data and props can declare the same property name. The order and
priority of the merging is not clear. Should a prop overwrite the field
with the same name in data? Or the other way around? Currently, the props'
initial values are available inside data functions, but the same field
returned in data gets overwritten by the prop.
2.

Once merged, it's not clear whether a component property is its
"private" state or a prop that is passed down from (and thus can be changed
by) the parent, unless you go and check the component's props declaration.

Proposal

Move props into its own name space, for example this.props. This avoids
the merging problem (we can either make props a reserved field or make it
$props) and makes it explicit that something is a prop that is obtained
from outside the component.


Reply to this email directly or view it on GitHub
https://github.com/yyx990803/vue/issues/1166.

mark-hahn commented Aug 17, 2015

In functions, vars passed via parameters and local variables are in the
same space. Vue should stay as it is.

On Sun, Aug 16, 2015 at 10:04 PM, Evan You notifications@github.com wrote:

Problem

Currently the props are merged with the instance's own data, which has a
few problems:

Both data and props can declare the same property name. The order and
priority of the merging is not clear. Should a prop overwrite the field
with the same name in data? Or the other way around? Currently, the props'
initial values are available inside data functions, but the same field
returned in data gets overwritten by the prop.
2.

Once merged, it's not clear whether a component property is its
"private" state or a prop that is passed down from (and thus can be changed
by) the parent, unless you go and check the component's props declaration.

Proposal

Move props into its own name space, for example this.props. This avoids
the merging problem (we can either make props a reserved field or make it
$props) and makes it explicit that something is a prop that is obtained
from outside the component.


Reply to this email directly or view it on GitHub
https://github.com/yyx990803/vue/issues/1166.

@mark-hahn

This comment has been minimized.

Show comment
Hide comment
@mark-hahn

mark-hahn Aug 17, 2015

Would this add more syntax to things like v-show? How about mustaches?

One of Vue's biggest features is its simplicity. I don't see how this proposal has enough benefit to excuse the added complexity. Things like removing $add are great.

"perfection is attained not when there is nothing more to add, but when there is nothing more to remove." - Antoine de Saint Exupéry

mark-hahn commented Aug 17, 2015

Would this add more syntax to things like v-show? How about mustaches?

One of Vue's biggest features is its simplicity. I don't see how this proposal has enough benefit to excuse the added complexity. Things like removing $add are great.

"perfection is attained not when there is nothing more to add, but when there is nothing more to remove." - Antoine de Saint Exupéry

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 Aug 17, 2015

Member

@mark-hahn Yes, this would make your template a bit more verbose. It's a tradeoff between conciseness (note - not necessarily simplicity) and explicitness. A component is stateful and its state can change over time - this is different from, say, a pure function call. It could be helpful to clearly know which part of the state is external and thus can change without the component itself doing anything.

That said, I'm also not 100% sure on this proposal yet. Would love to get more feedback from others.

Member

yyx990803 commented Aug 17, 2015

@mark-hahn Yes, this would make your template a bit more verbose. It's a tradeoff between conciseness (note - not necessarily simplicity) and explicitness. A component is stateful and its state can change over time - this is different from, say, a pure function call. It could be helpful to clearly know which part of the state is external and thus can change without the component itself doing anything.

That said, I'm also not 100% sure on this proposal yet. Would love to get more feedback from others.

@mark-hahn

This comment has been minimized.

Show comment
Hide comment
@mark-hahn

mark-hahn Aug 18, 2015

It could be helpful to clearly know which part of the state is external and thus can change without the component itself doing anything.

Objects passed into functions can also change without a function's help. You can't tell an object from a string using the syntax. Adding syntax for that, like hungarian notation, would be horrible.

Adding scope meaning to variables using syntax is a slippery slope. Should vars passed in to a function look different than local vars which look different than closure vars which look different than global vars?

mark-hahn commented Aug 18, 2015

It could be helpful to clearly know which part of the state is external and thus can change without the component itself doing anything.

Objects passed into functions can also change without a function's help. You can't tell an object from a string using the syntax. Adding syntax for that, like hungarian notation, would be horrible.

Adding scope meaning to variables using syntax is a slippery slope. Should vars passed in to a function look different than local vars which look different than closure vars which look different than global vars?

@Mat-Moo

This comment has been minimized.

Show comment
Hide comment
@Mat-Moo

Mat-Moo Aug 18, 2015

Contributor

I'm still not sure on this... When using 2 way bindings would it be data or a prop?

Contributor

Mat-Moo commented Aug 18, 2015

I'm still not sure on this... When using 2 way bindings would it be data or a prop?

@gamperl

This comment has been minimized.

Show comment
Hide comment
@gamperl

gamperl Aug 18, 2015

In my opinion Vue is one of the best frameworks I've ever seen because of its simplicity. Moving the params in its own scope, would make it's structure more complex. Why not show a warning if there is a param and a data property with the same name?

gamperl commented Aug 18, 2015

In my opinion Vue is one of the best frameworks I've ever seen because of its simplicity. Moving the params in its own scope, would make it's structure more complex. Why not show a warning if there is a param and a data property with the same name?

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 Aug 18, 2015

Member

@gamperl yeah, that could work too. Maybe something like "Don't redefine a prop in data. Use prop default value instead."

Member

yyx990803 commented Aug 18, 2015

@gamperl yeah, that could work too. Maybe something like "Don't redefine a prop in data. Use prop default value instead."

@JosephSilber

This comment has been minimized.

Show comment
Hide comment
@JosephSilber

JosephSilber Aug 19, 2015

I would actually like to have props separate. I do believe it keeps it all simpler.

JosephSilber commented Aug 19, 2015

I would actually like to have props separate. I do believe it keeps it all simpler.

@nirazul

This comment has been minimized.

Show comment
Hide comment
@nirazul

nirazul Aug 19, 2015

+1 for warning instead of different namespaces.
Most of the time you add to your own confusion when adding props and data attributes with the same name. There's no real use case for having props and data with same name, right?

nirazul commented Aug 19, 2015

+1 for warning instead of different namespaces.
Most of the time you add to your own confusion when adding props and data attributes with the same name. There's no real use case for having props and data with same name, right?

@simplesmiler

This comment has been minimized.

Show comment
Hide comment
@simplesmiler

simplesmiler Aug 19, 2015

Member

Warning sounds like a good idea.

Member

simplesmiler commented Aug 19, 2015

Warning sounds like a good idea.

@yyx990803 yyx990803 changed the title from Put props into its own namespace to Warn against duplicate props/data fields Aug 20, 2015

@karevn

This comment has been minimized.

Show comment
Hide comment
@karevn

karevn Aug 20, 2015

Contributor

Why not to embrace the probable interference taking in the next way:
"All the state info is 'data'. And 'props' is just a specification of the external interface available to the higher level component? Then, all the properties created by data function will become defaults for props passed from the outside.
I would also suggest to rename "props" to "attributes" to make it more straightforward - I see people are often confused in gitter discussions (and had some bad times myself).

Contributor

karevn commented Aug 20, 2015

Why not to embrace the probable interference taking in the next way:
"All the state info is 'data'. And 'props' is just a specification of the external interface available to the higher level component? Then, all the properties created by data function will become defaults for props passed from the outside.
I would also suggest to rename "props" to "attributes" to make it more straightforward - I see people are often confused in gitter discussions (and had some bad times myself).

@azamat-sharapov

This comment has been minimized.

Show comment
Hide comment
@azamat-sharapov

azamat-sharapov Aug 21, 2015

Contributor

It was called paramAttributes until 0.12 and was renamed to props later.
props sounds better for me and shorter to type.

Contributor

azamat-sharapov commented Aug 21, 2015

It was called paramAttributes until 0.12 and was renamed to props later.
props sounds better for me and shorter to type.

@davidkhess

This comment has been minimized.

Show comment
Hide comment
@davidkhess

davidkhess Aug 25, 2015

+1 for just emitting a warning on name clashes. The case of having a param and data attribute with the same name seems like it will lead to trouble and confusion.

davidkhess commented Aug 25, 2015

+1 for just emitting a warning on name clashes. The case of having a param and data attribute with the same name seems like it will lead to trouble and confusion.

@karevn

This comment has been minimized.

Show comment
Hide comment
@karevn

karevn Aug 25, 2015

Contributor

@azamat-sharapov why not to call it just attr then? One letter off. :)

Contributor

karevn commented Aug 25, 2015

@azamat-sharapov why not to call it just attr then? One letter off. :)

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 Aug 25, 2015

Member

@karevn no point in introducing breaking changes just out of naming preferences. props was objectively shorter and easier to type than paramAttributes, and people have gotten used to it now. attrs do not offer any substantial benefits to justify a change.

Member

yyx990803 commented Aug 25, 2015

@karevn no point in introducing breaking changes just out of naming preferences. props was objectively shorter and easier to type than paramAttributes, and people have gotten used to it now. attrs do not offer any substantial benefits to justify a change.

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 Aug 26, 2015

Member

Implemented in 1.0.0-alpha branch.

Member

yyx990803 commented Aug 26, 2015

Implemented in 1.0.0-alpha branch.

@yyx990803 yyx990803 closed this Aug 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment