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 "default" value based on other prop #106

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

prop "default" value based on other prop #106

mesqueeb opened this issue Dec 21, 2019 · 13 comments

Comments

@mesqueeb
Copy link
Contributor

mesqueeb commented Dec 21, 2019

Currently we can set default values like so:

export default {
  name: 'Knight',
  props: {
    name: {
      type: String,
    },
    isKnight: {
      type: Boolean,
      default: false,
    },
  },
}

In this example we'd have to pass if a person is a knight or not:

<Knight name="John" is-knight="false" />

However, very often I need to look at other props and calculate a props' default state based on these other props.

A nice addition to default values for props could be:

export default {
  name: 'Knight',
  props: {
    name: {
      type: String,
    },
    isKnight: {
      type: Boolean,
      default: props => props.name.includes('Sir'),
    },
  },
}
@posva
Copy link
Member

posva commented Dec 21, 2019

I think that as long as we can ensure key order in objects across all browsers, this is feasible and it aligns with function usage:

// both work
function something(a, b = a * 2, c = b * 2) {}
function something({ a, b = a * 2, c = b * 2) {}

which would make sense as props are to components what arguments are to functions

@michaeldrotar
Copy link

michaeldrotar commented Dec 21, 2019

I don't think this would work... taking the example further:

export default {
  name: 'Knight',
  props: {
    name: {
      type: String,
      default: props => props.isKnight ? 'Sir Lancelot' : 'Lady Lancelot'
    },
    isKnight: {
      type: Boolean,
      default: props => props.name.includes('Sir'),
    },
  },
}

props.name would be undefined if isKnight is evaluated first, but it'd be Lady Lancelot if it's evaluated second (by virtue of isKnight still being undefined and thus being falsey -- which itself is not entirely logical).

If props.name is evaluated when isKnight tries to access it, then there would be a circular reference.

I think you need to decide if isKnight is really a prop or if it's a computed... and if you want something that's both -- well I haven't found a great pattern for that in Vue yet short of having separate thing and computedThing and knowing when to use which... maybe someone else can share if there's a better solution.

@mesqueeb
Copy link
Contributor Author

@michaeldrotar
Yes it will crash when they are referencing each other, but so do computed props referencing each other.

@cawa-93
Copy link

cawa-93 commented Dec 24, 2019

In my opinion, the default value should not depend on anything else. It should be simple and clear. For everything else, there are computed properties

For example, your problem should be solved as follows:

export default {
  name: 'Knight',
  props: {
    name: {
      type: String,
      default: 'Anonym'
    },
    isKnightProp: {
      type: Boolean,
    },
  },

  computed() {
    isKnight() {
      return this.isKnightProp !== undefined ? this.isKnightProp : this.name.includes('Sir'),
    }
  }
}

This example is easier to read and understand by a third-party developer.

UPD
In addition, it may be possible when used Composition API

@thedamon
Copy link

thedamon commented Jan 2, 2020

I don't think it's a foregone conclusion that computed props are easier to understand than dependent prop defaults. Using a computed prop is logically straightforward from a code point of view but conceptually complex and introduces a lot of unpredictability

Firstly and mainly, as mentioned, it matches native javascript's own native function behaviour where function parameter defaults can use previous arguments, and thinking of components as functions and props as parameters is a pretty reasonable mental model.

If we're relying on a computed to do this, the mental model of the component becomes more confusing, because you have to name a computed prop that is conceptually identical to the prop it reflects.

In the code @cawa-93 posted it's isKnightProp and isKnight.. but I don't think naming a prop based on internal implementation details of the component is an ideal pattern. That means if I already have this component without the 'sir' check and I want to add it, I need to rename the prop which is a much larger more breaking change than adding a new computed prop. It also doesn't make sense to a user of the component why some props have the word Prop after them.

So if we want to keep the API of the component "clean".. what do you call the isKnight computed prop? isActuallyKnight? computedIsKnight? _isKnight? all of these names are silly, because the mental model of the computed property is exactly that of the prop itself: isKnight. And to be really nitpicky.. the time spent deciding what to name this computed property, or whether to rename the prop or the computed, is completely unnecessary with the functionality suggested here.

If we go with a weirdly named computed property instead of a weirdly named prop, developers will have to know to always reference the computed prop rather than the 'smart' prop itself which is counterintuitive. So really, renaming the prop makes more sense from an internal point of view, but less sense from an external point of view — nobody wins.

If we could just reference a prop in a default the entire conundrum vanishes, and it doesn't really 'change' anything, so much as it removes a current limitation. We already have default as a prop option, and it's even potentially confusing that you can't reference other props there. We also already have to define default as a function if we want to be able to use $t to have a translated default String prop. (buttonLabel: {type: String, default: function(){return this.$t('buttons.close')}})

This solution is elegant because it doesn't really need its own syntax or API*, just a change to how props are evaluated, and suddenly we have a new ability to express our component logic more succinctly (and if it seems 'wrong' to do it, there can be an Eslint rule called noDependentPropDefaults or something).

* If it's complex/slow to force evaluation order of props to be based on component options instead of template, then adding an additional prop option that makes the prop evaluated later could be a workaround, but maybe making prop evaluation follow order of prop declaration rather than the template would be better (taking into account props passed through v-bind as well).. that might be a way to backport it into v2 more easily.

@michaeldrotar
Copy link

Agree with the confusing names.. that's something I've been struggling with in Vue recently and ties into a similar thread that was recently opened about how to handle casting.

I think Ember actually solves this better.. Ember doesn't differentiate what can be passed to a component vs internal data... this can have downsides, but it also lets you simply have a computed with a getter and setter and whether the value is passed externally or handled internally it doesn't matter.. you can do any casting/converting/defaulting without having to have differently named versions of the same thing.

So to me.. that's the missing piece. I guess I want computeds to be externally settable? Or I want props to have computed functionality?

props: {
  isKnight: {
    type: Boolean,
    getter(self) { // using self instead of just `props` cause maybe you need computeds or methods too?
      return self._isKnight !== undefined ? self._isKnight : self.name.includes('Sir');
    },
    setter(self, value) {
      self._isKnight = value;
    }
  },
  age: {
    type: Number,
    getter(self) {
      return self._age;
    },
    setter(self, value) {
      self._age = Number(value);
      if (isNaN(self._age) || self._age < 0) {
        self._age = undefined;
      }
    }
}

Or an alternate pattern... if we assume the return value of setter is a cached value that Vue tracks and uses then getter is only called if nothing is set, so it becomes really a default like OP has

props: {
  isKnight: {
    type: Boolean,
    default(self) {
      return self.name.includes('Sir'); // re-compute when `name` dep changes.. once explicitly set, this is never called again
    }
  },
  age: {
    type: Number,
    setter(self, value) { // maybe a better name than `setter`
      const age = Number(value);
      if (isNaN(age) || age < 0) {
        return undefined;
      }
      return age;
    }
}

@Meekohi
Copy link

Meekohi commented Jan 2, 2020

I assume you don't want a simple isKnight computed property, because it should be editable vs reactive -- shouldn't that just go in data then? I tend to name these types of props init-*:

data: {
  isKnight: this.initKnight || this.name.includes('Sir'),
},
props: {
  name: String,
  initKnight: {
    type: Boolean,
    default: false
  },
}

@thedamon
Copy link

thedamon commented Jan 3, 2020

@michaeldrotar what you describe sounds a lot like the parse suggestion discussed here (and also known as coercion) which does solve a rather similar problem.

It's an interesting approach and potentially powerful.. (it would certainly solve everything I'm concerned about) but part of me also thinks that it adds a level of confusion in that the props you pass in can be completely butchered, and that while useful does seem like a potential reduction in simplicity. (ie isKnight.setter()=>'grabblebibbly').

I would be happy to see that feature though.. but there is a lot of overlap with computed (and is there a reason coercion is synonymous with lying and generally frowned upon culturally? 😅).

Basing defaults off other props doesn't seem like overlap with computed and doesn't actually change the API, so it seems rather nice.

@Meekohi that is an interesting approach, though my main beef is the external prop called initKnight.. it seems like odd naming for a public API when really what it means is isKnight. Also brings up, what should happen if the default is based on a prop but the prop changes... in that way @michaeldrotar 's suggestion of a separately evaluated setter seems more explicit and theoretically more likely to be what developers want (otherwise it might make sense to re-evaluate any default function whenever dependent props change)

@thedamon
Copy link

thedamon commented Jan 3, 2020

I think it is important for this conversation to add that this is currently already possible to try to do. But it is not supported (I think?).

Here is a demonstration of doing it where it seems to be working even though I thought #4 would not work, because my understanding was that props are evaluated in the order they are passed to the component.

So maybe we could avoid all this conceptual back and forth and do it and call it a bug fix if there are cases where it doesn't work as expected? 🤔(I could have sworn there were, I just can't repro it not working right now.)

@backbone87
Copy link

i think the 1:1 comparism with function calls does not fit. while arguments to functions have a strict order that is represented in all its serializations (parameter list on definiton, argument list on call), this is not the case for props, which have no explicit orders:

  • "call" serialization (HTML, JSX): it is well known that order of attributes in HTML has no significance
  • "define" serialization: hashes without order guarantees are used (object literals)

@michaeldrotar
Copy link

@thedamon - Agree coercion could be abused, but in practice I've never had that issue. It'd be like calling isEven(6) and getting "apricot" as a response... like people can do it but I think it's good enough to say not to.

As far as simplicity... I'm torn. Again, I agree on the theory.. but if the "simpler" solution is to have a isKnight prop and a isKnightComputed computed.. and to have to remember to only use the isKnightComputed internally and never use the isKnight prop cause the computed handles some edge case coercion.. then that feels like more mental juggling.

Common scenario: I'm on a large team where maybe it used to be isKnight, later gets superceded by isKnightComputed and then someone else didn't realize the computed version was added and uses the prop version again. The prop version works 90% of the time but not in some edge case that the computed was made for.. like maybe when interacting with some 3rd party lib that passes an input value as a string "false" and the computed handles converting it to false but using the prop version is still truthy.


As to computeds providing the same functionality.. totally agree, issue is that they can't be assigned externally like props. So my other idea was to let computeds be assignable (like Ember does)..

(This assumes computeds work.. or could work.. the same as how I proposed earlier.. that they only call get/default when not assigned and then cache the return value of set as the value.)

computed: {
  isKnight: {
    prop: true, // needs a better name but something to say it's assignable like other props
    get: (self) => self.name.includes('Sir') // trying a non-this syntax
  },
  age: {
    prop: true,
    set(self, value) {
      const age = Number(value);
      if (isNaN(age) || age < 0) {
        return undefined;
      }
      return age;
    }
}

Personally I think this is fine too, but it does mean having two places to check in order to know the full scope of what can be passed in... which isn't ideal.


Here's an off-the-wall idea.. again inspired by Ember... maybe computed being a section is too limiting and it should be more of a function that can be applied

data: function() {
  return {
    someNumber: 1,
    somePlural: computed((self) => self.someNumber === 1 ? 'thing' : 'things')
  };
},
props: {
  isKnight: computed((self) => self.name.includes('Sir'))
}

@thedamon
Copy link

thedamon commented Jan 5, 2020

I kinda love the potential ability of having computed as an option for props that creates a computed function property that supersedes the prop internally (but probably without the setter option). That makes it clear that it will behave like a computed prop but keeps the definitions paired nicely.

(Though allowing default to be a function achieves almost the same thing, but it’s not clear if it will be re-evaluated when the prop changes... or rather logically it wouldn’t but practically it probably should). (The tricky part of the “working” example I posted above is that it never recomputes))

@thedamon
Copy link

thedamon commented Jan 6, 2020

This use case is actually also solved by the suggestion to add 'as' as a prop option for internal naming.

But is it also worth addressing that this already sometimes works (at least in Vue 2) and is necessary for using i18n inside a string default? I think the current behaviour should either work, or not work, or should maybe be added to default eslint

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