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

prop "parse" function #107

Open
mesqueeb opened this issue Dec 21, 2019 · 10 comments
Open

prop "parse" function #107

mesqueeb opened this issue Dec 21, 2019 · 10 comments

Comments

@mesqueeb
Copy link
Contributor

mesqueeb commented Dec 21, 2019

I think it would save a lot of boilerplate code if we could somehow transform props with a simple function defined right in the props configuration.

Eg. a function called parse could be executed before props are passed to setup(props) and before the props are registered on this

export default {
  props: {
    name: {
      type: String,
      parse: val => titleCase(val)
    },
    skills: {
      type: Object,
      // merge onto default values
      parse: val => Object.assign({power: 9000}, val)
    },
  },
}

The current implementation is:

export default {
  props: {
    name: String,
    skills: Object,
  },
  data () {
    const nameParsed = titleCase(this.name)
    const skillsParsed = Object.assign({power: 9000}, this.skills)
    return { nameParsed, skillsParsed }
  },
  watch: {
    name (val) {
      this.name = titleCase(val)
    },
    skills (val) {
      this.skills = Object.assign({power: 9000}, val)
    },
  },
}

The main problems are:

  • name clashing forces us to use a convention like nameParsed
  • we need to set up additional watchers, just for simple parsing of props
  • we need to decide if it's not better to do all this as computed property or not (i honestly don't know the trade-offs between computed and data + watch)

A simple parse function to be defined in the prop config seems like a very valuable addition. It's easy to understand and not breaking; so backwards compatible.

@smolinari
Copy link
Contributor

Not saying this idea isn't viable, just trying to play devil's advocate. 😄

we need to decide if it's not better to do all this as computed property or not (i honestly don't know the trade-offs between computed and data + watch)

The trade off is, AFAIK, watchers should be mainly reserved for more asynchronous updating of the data.

Not sure why your example prop parsing can't be done via computed props and why it isn't a simple decision to do so. Computeds are there for exactly that purpose. Parsing or rather recalculating values, which is what both your examples do.

Sure, there is still the name clash issue of sorts. You just need to find a proper name for your computeds (and I'm not saying the ones below are great either 😁 ).

  computed: {
    nameTitleCased () {
      return titleCase(this.name)
    },
    skillsComputed () {
      return Object.assign({power: 9000}, this.skills)
    }

Or, since this is purely internal to the component, just put an underscore in front of the prop name. So _name and _skills.

The other, I believe, possibly viable alternative yet not wondrously great is offering a constructor function as the prop type, so outside of the component, the prop must be built via the constructor function. This constructor function can then also take over the parsing bit. That seems a bit more difficult though from a devland perspective, because it might not be clear upfront about how the prop needs to be "built via new" in order to pass the validation check. But, it should work (never done it myself to be honest).

https://vuejs.org/v2/guide/components-props.html?#Type-Checks (the last bit about the constructor function).

Scott

@posva
Copy link
Member

posva commented Dec 21, 2019

This has been discussed previously in multiple issues, one of them is vuejs/vue#8049. By looking at the download stats of the package that implement such behavior, we can see that most people don't have the need, or at least do not suffer from using computed properties in order to justify adding a package to add such functionality.

Among the problems you mention, the only one that exists is the need to have multiple names but ending up only using one (the sanitized version). Using a computed should always be the way to go, never a watcher that sets a data because those are meant for side effects while prop sanitation should be a pure function.

@desislavsd
Copy link

desislavsd commented Dec 21, 2019

@posva the package you mentioned does not look nearly as satisfying as the proposed feature. Also computed are not enough to deal with some props. Generally I imagine that props might follow much more flexible convention:

  • there should be a parse function instead of default option for converting the prop value in to the appropriate format for an internal usage

  • this internal value should be seen as a local variable that one may update at anytime without affecting the prop's original value in the parent component

  • yet each time the prop is updated in the parent component the parse function is triggered and thus updates the internal value

  • and of course one should stick with $emit('update:...', ...) and .sync modifier in order to update original prop value and the internal value in sync.

As mentioned by @mesqueeb the current implementation involves the definition of a local data property and a watcher:

export default {
    props: ['foo'],
    data(){
        
        // I need 'foo' properly formatted
        let _foo = this.parseFoo(this.foo);
        
        return { 
            _foo, 
            // 'bar' definition depends on the initial value of 'foo'
            bar: !!_foo
        }
    },
    watch: {
        // I need '_foo' updated each time 'foo' changes
        foo(){
            // the following code line already exists in the `data()`
            this.$data._foo = this.parseFoo(this.foo) 
        }
	},
    methods: {
        parseFoo(){
            return this.foo
		},
        updateFoo(val, sync){
          
           	// sometimes I update local value
            // through updating parent's value
            if( !sync ) 
                return this.$data._foo = val;
            
            // sometimes I don't want to affect the parent;
            this.$emit('update:foo', val)
        },
    }
    
}

Notice how we repeat code to set _foo both in the data() method and in the watcher. A watcher with immediate: true won't help as we need the initial value of foo in data() for the definition of the _bar property. When making complex reusable components ( potentially vue packages ) I often struggle with similar scenarios. And the naming problem is quite a reason by itself 😊.

To me this non breaking feature looks neat and useful. Also I believe it is really easy to implement and is a matter of a just a few lines of code, so I hope the team finds it convenient.

@posva
Copy link
Member

posva commented Dec 21, 2019

@posva the package you mentioned does not look nearly as satisfying as the proposed feature.

The only thing missing is being able to rename the prop, which is the only problem. There is an open issue for that btw: vuejs/vue#7943

Also computed are not enough to deal with some props

I don't see how computed couldn't be enough. What are you thinking about

Regarding the whole watcher thing, a computed property to derive data from a prop is the way to go. Watchers are for side effects, as mentioned in the previous comment

@desislavsd
Copy link

The only thing missing is..

Its not about what is missing in the package, Its about API design. The API it provides is not simple enough for the functionality it covers.

@posva
Copy link
Member

posva commented Dec 21, 2019

Its not about what is missing in the package, Its about API design

It's about more than that to be fair but the API could be the same if the mentioned issue was implemented. It also shows more interest by the community and would allow the prop coercion feature to be implemented in user land to benefit those who prefer that syntax. The main reason prop coercion was removed is because it was redundant with computed properties

@bbugh
Copy link

bbugh commented Dec 22, 2019

In #78, @nekosaur said in this comment:

Also be aware that you will not have the issue of clashing prop/setup names in Vue 3, that's only an issue with @vue/composition-api.

If I'm reading that correctly, in Vue 3 with the composition api, we'll be able to wrap props and return a new one with the same name without Vue raising an error. Can anyone confirm that? Something like this:

createComponent({
  props: {
    foo: Number
  },
  setup (props) {
    const foo = computed(() => props.foo * 2

    // ❌ Vue 2 with composition api causes an error
    // ❓ Vue 3 ...works? 
    return { foo }
  }

@thedamon
Copy link

thedamon commented Jan 3, 2020

Though not mentioned in the description, I think it's worth noting that this suggestion would essentially solve the issue behind #106

@reed-jones
Copy link

I was originally in favor of this change, however after reading through #10 it seems to me too be a better solution (combined with a computed prop). As it keeps both the raw prop value, and the "parsed" value.

export default {
  props: {
    name: {
      type: String,
      as: 'rawName'
    },
  },
  computed: {
    name() {
      return titleCase(this.rawName)
    }
  }
}

@smolinari
Copy link
Contributor

smolinari commented Jan 3, 2020

Yeah, agree. Parsing/ recalculation use computeds. Then implement the "as" solution for internal naming. 👍

Scott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants