Custom default values for the Values API #350

marcoroth · 2020-12-12T04:28:20Z

Hey there!

I teamed up with @leastbad to implement custom default values for the Values API as discussed in #335.

The original syntax was proposed by @dancallaghan here and enhanced by @paulozoom here.

You can still define your values with the type, so the syntax is fully backwards-compatible. Also I've added some tests to cover the most common scenarios.

Example

Custom default values are perfect so you don't need to write getters (or similar constructs) like this:

// demo_controller.js

import { Controller } from 'stimulus'

export default class extends Controller {
  static values = {
    url: String,
    interval: Number,
    params: Object,
    clicked: Boolean,
    ids: Array
  }

  connect () {
    console.log(this.url, this.interval, this.params, this.clicked, this.ids)
  }
  
  get url() {
    return this.urlValue || '/bill'
  }
  
  get interval() {
    return this.intervalValue || 5
  }
  
  get params() {
    return this.paramsValue || { name: 'sue' }
  }
 
 get clicked() {
    return this.clickedValue || true
  }

  get ids() {
    return this.idsValue || [1]
  }
}

// "/bill" - 5 - { name: "sue" } - true - [1]

The above example can be simplified to this:

// demo_controller.js

import { Controller } from 'stimulus'

export default class extends Controller {
  static values = {
    url: '/bill',
    interval: 5,
    params: { name: 'sue' },
    clicked: true,
    ids: [1]
  }

  connect () {
    console.log(this.urlValue, this.intervalValue, this.paramsValue, this.clickedValue, this.idsValue)
  }
}

// "/bill" - 5 - { name: "sue" } - true - [1]

Override custom default values

You can override the custom default values as you already do with the current Value API.

<div data-controller="demo"
  data-demo-url-value="/jack"
  data-demo-interval-value="10"
  data-demo-params-value='{ "name": "ruby" }'
  data-demo-clicked-value="false"
  data-demo-ids-value="[2, 3, 4]">
</div>

We are happy to add docs as well if the implementation and the syntax is fine! Thank you!

Resolves #335.

If you define a custom default value in your controller the `has[name]Value` method should return `true`. This is useful when you have behavior like this in your controller and want to differentiate between a custom default value and the regular default value: import { Controller } from 'stimulus' export default class extends Controller { static values = { refreshInterval: 500 } connect() { if (this.hasRefreshIntervalValue) { this.startRefreshing() } } } So with this change the above code will be executed because you have a custom default value set.

leastbad · 2020-12-12T04:48:07Z

Excellent work, @marcoroth - who frankly assigns me too much credit, as this is a much-needed re-write in proper TypeScript with tests.

@dhh @javan hopefully this just slides like a hand in glove. 🧤

dancallaghan · 2020-12-12T08:16:30Z

Nice work 👍

javan · 2020-12-14T17:56:56Z

Thank you! Really appreciate how thorough this implementation is.

I'm not totally sold on using the default value to implicitly specify its type and the fact that it's a default in the first place. Borrowing from the sample from above:

export default class extends Controller {
  static values = {
    url: '/bill',
    interval: 5,
    clicked: Boolean,
  }
}

There's mental overhead involved to understand a) the data attribute to type conversion that will happen, and b) that the first two are defaults, not constants (this.urlValue won't necessarily be '/bill').

Let's compare with the syntax originally proposed in #335:

export default class extends Controller {
  static values = {
    url: [ String, '/bill' ],
    interval: [ Number, 5 ],
    clicked: Boolean,
  }
}

And here's a third option that spells it all out:

export default class extends Controller {
  static values = {
    url: { type: String, default: '/bill' },
    interval: { type: Number, default: 5 },
    clicked: Boolean,
  }
}

Obviously the first is shorter, but if you asked me to explain what it does without first knowing exactly how Stimulus works, I don't think I could. The second one: maybe. The third: probably.

jaredcwhite · 2020-12-14T20:19:55Z

@javan I would probably vote that third option as well…it's very similar to property descriptors in other libraries that I've seen, which isn't per se a reason to choose it, but it does mean it's already familiar and obvious.

leastbad · 2020-12-14T20:31:13Z

For context, a quick positive consensus grew on #335 around the first option, which I admit I'm still partial to... but I originally proposed option two (aka @dancallaghan's take) and would be very happy with that syntax.

I find option three to be unnecessarily verbose, given that it's almost as long as the code we're trying to disappear. I think that anyone who wants to use this feature could learn any of the three proposed forms very quickly - default values are not an exotic concept.

I'm not super convinced of the "explain what it does without knowing how Stimulus works" argument in this context - you could say that about anything you haven't read the (very simple and straight-forward) docs on, yet.

dancallaghan · 2020-12-14T20:49:20Z

I was also thinking yesterday about the possibility of adding required values, which would be hamstrung by the terse syntax. I feel like going down this path would stymie things in the future or lead to having multiple idiosyncratic syntaxes for different scenarios.

Adding required values would be easy with the verbose syntax:

export default class extends Controller {
  static values = {
    url: { type: String, required: true },
    interval: { type: Number, default: 5 },
    clicked: Boolean,
  }

  connect() {
    console.log(this.urlValue); // This would throw an error á là this.missingTarget
  }
}

I tend to agree with @javan that there's a bit too much magic/implied knowledge going on.

Edit: realised having a default value and required on the same definition was dumb

paulozoom · 2020-12-14T20:54:40Z

I haven't contributed with anything other than the shorter syntax suggestion of option one, after seeing @leastbad’s original syntax. I did so because it felt somewhat Rails-like in its elegance even if at a slight cost of explicitness.

That said, I feel both option 1 and 3 are fine choices based on two different philosophies/approaches, and I think it’s up to the core team to pick which one they feel fits the overall project better.

julianrubisch · 2020-12-16T18:24:27Z

I, too, would vote for option 3, even if we‘re approaching typescript syntax fast 😅

DamnedScholar · 2020-12-16T22:35:06Z

Edit: realised having a default value and required on the same definition was dumb

Is it? That might be useful for a value that can change but should cause the controller to at least complain in the console if it disappears or becomes falsey.

dancallaghan · 2020-12-16T22:51:07Z

I did so because it felt somewhat Rails-like in its elegance even if at a slight cost of explicitness

I would disagree with this being Rails-like, as in Rails-land most options are hashes. The closest parallel I could think of is the attribute class method, which explicitly passes default: 'foo' as an option.

Is it? That might be useful for a value that can change but should cause the controller to at least complain in the console if it disappears or becomes falsey.

I guess it depends on how required is handled. If it throws an error (which is how I imagined it to work), they would be mutually exclusive. If it's just a warning, then they could happily coexist.

leastbad · 2020-12-16T23:54:12Z

We're going way, way down a rabbit hole, folks. Why stop at required fields with errors when you could have a full validation DSL, after all? I kid out of love, but aren't we best served with a simple, efficient syntax?

What we have to do today:

static values = {
  padding: Number
}
if (!this.hasPaddingValue) this.paddingValue = 10

What we could have:

static values = {
  padding: [Number, 10]
}

What I'm hoping to avoid:

static values = {
  padding: { type: Number, default: 10 }
}

If padding: Number was enough and didn't need to be padding: {type: Number} to be understood, I don't get why we suddenly need an object for this.

Stimulus documentation is consistently great, and I expect padding: [Number, 10] would be equally well explained.

adrienpoly · 2020-12-17T06:51:46Z

I have seen quite a few examples in other libraries where params are either a simple array for the most basic cases or an Object to allow for more advanced settings.
While it is a slightly more verbose option 3 feels quite natural and explicit to me, and I like it 👍 .

I see another benefit with option 3 as it allows more options down the road.
In the initial PR we discussed a custom handler for parsing values. While the idea looked appealing at first sight no obvious use cases were listed at that time.

Looking at #353 maybe it is an example where rather than having a json5 parser for all to solve the issue (json5 is 9k gzip so it would double the size of Stimulus) we could through a special parser include it at the application level.

The object syntax would make this much easier

static values = {
  customParser: { parser: (value) => json5.parse(value), default: [] }
}

darylbarnes · 2020-12-17T10:27:25Z

I’ve just started using Stimulus and I’m more of an HTML/CSS guy than a JavaScript expert and I think option 1 is what I would use most of the time. I don’t feel like being explicit here is all that important. However, option 3 does give you more options down the road and that simplicity shouldn't cause problems when our needs become more complex. So if we just rule out option 2, isn’t it possible for Stimulus to allow use of either option 1 or 3 by requiring the type and default if option 3 is used? export default class extends Controller { static values = { url: '/bill', interval: 5, clicked: Boolean, } } export default class extends Controller { static values = { url: { type: String, default: '/bill' }, interval: { type: Number, default: 5 }, clicked: {type: Boolean, default: false } } } I would like consistency and being explicit when option 3 is used with no magic involved so even for Boolean it should be declared. To declare which option you’re using, you could also do this when Option 3 is used: export default class extends Controller { static typeValues = { url: { type: String, default: '/bill’ }, interval: { type: Number, default: 5 }, clicked: {type: Boolean, default: false } } } I’m no JavaScript expert by any means though so take my advice with a grain of salt. ;-)

…

On Dec 17, 2020, at 12:52 AM, Adrien Poly ***@***.***> wrote: I have seen quite a few examples in other libraries where params are either a simple array for the most basic cases or an Object to allow for more advanced settings. While it is a slightly more verbose option 3 feels quite natural and explicit to me, and I like it 👍 . I see another benefit with option 3 as it allows more options down the road. In the initial PR we discussed a custom handler for parsing values <#202 (comment)>. While the idea looked appealing at first sight no obvious use cases were listed at that time. Looking at #353 <#353> maybe it is an example where rather than having a json5 parser for all to solve the issue (json5 is 9k gzip so it would double the size of Stimulus) we could through a special parser include it at the application level. The object syntax would make this much easier static values = { customParser: { parser: (value) => json5.parse(value), default: [] } } — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#350 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXKF77XJTVEK6ZUGOLNV5LSVGTBVANCNFSM4UXVTYAA>.

cabgfx · 2020-12-17T20:06:54Z

I'm in the same boat as @darylbarnes, and would probably also use Option 1 most of the time.
I guess if I saw this:

export default class extends Controller {
  static values = {
    url: '/bill',
    interval: 5,
    clicked: Boolean,
  }
}

I'd assume those to be defaults. And then having a verbose Option 3 available as well makes perfect sense to me, coming from mostly jQuery, where this kind of signature is prevalent.

inopinatus · 2021-01-02T05:09:57Z

It'd be in keeping with Stimulus's overall structure if the values & classes API delegated the default back to the controller. Might be helpful when subclassing controllers, too.

leehericks · 2021-03-30T04:55:40Z

@adrienpoly @dancallaghan @javan
Big picture tying in with #249.

Incredibly explicit and concise; easy to teach and simply add a defaults section to each part of the reference.

// Class default value could just be “[controller]-loading”
static classes = [ “loading” ]

static values = {
  padding: Number
}

// Defaults (overrideable and consistent)

get defaultLoadingClass() {
  return “render-loading”
}

get defaultPaddingValue() {
  return 10
}

get defaultUrlParam() {
  return “/render”
}

// Actions

render(event) {
  // Event params property includes defaults
  // and can also use event.hasUrlParam

  // Even though the type here is unused, 
  // it is consistent and self-documenting
  const { url: String } = event.params

  // paddingValue will fallback to default if exists
  ... this.paddingValue
}

leehericks · 2021-03-30T08:51:52Z

@marcoroth I'm sorry, I know this doesn't follow the syntax you created for this great pull request. I'm just weighing in after many hours of considering the event params api and the conversation that took place in that PR along with the conversation in this one, particularly responses that came from core members in both.

It'd be in keeping with Stimulus's overall structure if the values & classes API delegated the default back to the controller. Might be helpful when subclassing controllers, too.

This is a valid point from @inopinatus. If you heavily overload the values and classes declarations then you reduce the potential for subclassing. In my example, the subclass can simply declare new defaults.

Thank you! Really appreciate how thorough this implementation is.

I'm not totally sold on using the default value to implicitly specify its type and the fact that it's a default in the first place. Borrowing from the sample from above:
export default class extends Controller {
  static values = {
    url: '/bill',
    interval: 5,
    clicked: Boolean,
  }
}
There's mental overhead involved to understand a) the data attribute to type conversion that will happen, and b) that the first two are defaults, not constants (this.urlValue won't necessarily be '/bill').

Let's compare with the syntax originally proposed in #335:
export default class extends Controller {
  static values = {
    url: [ String, '/bill' ],
    interval: [ Number, 5 ],
    clicked: Boolean,
  }
}
And here's a third option that spells it all out:
export default class extends Controller {
  static values = {
    url: { type: String, default: '/bill' },
    interval: { type: Number, default: 5 },
    clicked: Boolean,
  }
}
Obviously the first is shorter, but if you asked me to explain what it does without first knowing exactly how Stimulus works, I don't think I could. The second one: maybe. The third: probably.

I agree with this response from @javan whole-heartedly. Keeping the simple type declaration is incredibly self-documenting and easy to understand. The third option is explicitly defined at the expense of a complex declaration.

The same simple declarations can be used for the event params API as well, so we can share controllers and know we just look at the top of the action to see if params are declared and what type the author expects them to be.

render(event) {
  const { url: String } = event.params
}

Stimulus relies heavily on generated properties. That's why default properties fit right in. We can easily add and teach expectations about naming that is consistent across the framework. It's already easy to see how the defaults feature can cleanly be added to the Stimulus Reference as well.

Controller.values
this.urlValue
this.urlValue=
this.defaultUrlValue
this.hasUrlValue

Controller.classes
this.loadingClass
// Stimulus could generate this to return "[controller]-loading"
// and that would still let you define your own defaultLoadingClass
this.defaultLoadingClass
this.hasLoadingClass

event.params
event.defaultUrlParam
event.hasUrlParam

Finally, if you declare the values and default like you are proposing,

static values = {
    url: '/bill',
    interval: 5,
    params: { name: 'sue' },
    clicked: true,
    ids: [1]
  }

then Stimulus will need to generate this default properties anyway, correct?

you will use hasUrlValue to know if they added a value in the dataset
you will use urlValue to get the value from the dataset or the default value if it exists or an error
you may want to access the default value in your code for fallback, defaultUrlValue, I guess returning undefined if you didn't provide a default in the declaration.

So I guess that makes it just syntactic sugar that reduces the need for many get default[name]Value and it will fall on the core team to decide if they want to keep the very clean syntax.

Sorry again, I come from a lot of Apple coding where APIs try to favor readability and I'm ADHD, so that's why I really love Stimulus as it is and how simple Hotwire made it to add in page replacement with one turboframe tag.

marcoroth · 2021-03-31T19:23:16Z

I really appreciate everyone's input on this concept. There are few things more flattering than spirited debate over the merits of different potential implementations.

Now that a few months have passed and we've had an opportunity to think through the implications of each variation, it seems like the opinions are generally split between what @javan referred to as "option 1" and "option 3".

What I've done is run with @darylbarnes clever idea to support both, allowing us to enjoy the best of both worlds.

One of the reasons I really like option 1 is because it's more expressive and natural in my opinion. Even if you just use the regular default values instead of the explict types. Often times, especially in the case of Array, Object and Boolean, I feel like it's more intuitive to see the actual value instead of the type.

In this example you can very easily recogonize what kind of value it is, plus it feels very JS-like:

export default class extends Controller {
  static values = {
    features: [],
    person: {},
    enabled: false
  }
}

You don't need to lookup or translate the default value for each type in your head, it's just written there - without the need to define a type. I think it then just feels as intuitive and natural to also set a non-default value that way.

If you don't prefer this version you can still fallback to the more explicit/verbose option 3. The same example would look like this:

export default class extends Controller {
  static values = {
    features: { type: Array, default: [] },
    person: { type: Object, default: {} },
    enabled: { type: Boolean, default: false }
  }
}

I'm really happy with this solution and very excited to support both options ☺️

dhh · 2021-06-13T07:56:16Z

I concur with Javan's analysis. Most values do not need defaults, so they get the shortest, yet plainest form. Values that do need defaults use the expanded, but clearest form. Also, the example above makes the long-form look needlessly cumbersome because the defaults don't add anything. Defaults need different values than empty containers to make sense.

Anyway, I'd be happy to see us proceed with this form:

export default class extends Controller {
  static values = {
    url: { type: String, default: '/bill' },
    interval: { type: Number, default: 5 },
    clicked: Boolean,
  }
}

leastbad and others added 7 commits December 10, 2020 09:34

default values

af84ded

Update value_properties.ts

f8093ad

dry up ValueProperties#parseValueDefinitionPair and use explicit types

3313ad6

introduce ValueTypeDefinition type

6bdb75e

update naming of the parse functions

7117a10

add default values tests

da3bd29

Base automatically changed from master to main January 12, 2021 20:57

adrienpoly mentioned this pull request Feb 2, 2021

Support Stimulus 2.0 Values API adrienpoly/stimulus-flatpickr#69

Open

marcoroth added 2 commits March 31, 2021 17:30

Merge remote-tracking branch 'upstream/main' into default_values

6f53805

allow to set default values via a ValueTypeObject

0b91ac0

dhh mentioned this pull request Jun 13, 2021

Values API default values #335

Closed

marcoroth added 2 commits July 1, 2021 10:08

Merge branch 'main' into default_values

28bd81f

fix compiler errors

61877f7

marcoroth force-pushed the default_values branch from 925f70f to 61877f7 Compare July 1, 2021 08:13

marcoroth added 4 commits July 2, 2021 15:56

Merge branch 'hotwired:main' into default_values

33a55b4

use writer instead of as string

fb56e13

adapt value changed callbacks to take custom default values into account

d3822aa

invokeChangedCallbackForValue -> invokeChangedCallback

8945f6b

dhh merged commit 785af9a into hotwired:main Jul 3, 2021

dhh mentioned this pull request Jul 3, 2021

v2 Proposed Enhancement: fooTargetsChanged callback #336

Closed

marcoroth mentioned this pull request Sep 2, 2022

Value defaults not committed to DOM #580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom default values for the Values API #350

Custom default values for the Values API #350

marcoroth commented Dec 12, 2020 •

edited

Loading

leastbad commented Dec 12, 2020

dancallaghan commented Dec 12, 2020

javan commented Dec 14, 2020 •

edited

Loading

jaredcwhite commented Dec 14, 2020

leastbad commented Dec 14, 2020

dancallaghan commented Dec 14, 2020 •

edited

Loading

paulozoom commented Dec 14, 2020 •

edited

Loading

julianrubisch commented Dec 16, 2020

DamnedScholar commented Dec 16, 2020

dancallaghan commented Dec 16, 2020

leastbad commented Dec 16, 2020

adrienpoly commented Dec 17, 2020

darylbarnes commented Dec 17, 2020 via email

cabgfx commented Dec 17, 2020

inopinatus commented Jan 2, 2021

leehericks commented Mar 30, 2021 •

edited

Loading

leehericks commented Mar 30, 2021 •

edited

Loading

marcoroth commented Mar 31, 2021

dhh commented Jun 13, 2021 •

edited

Loading

Custom default values for the Values API #350

Custom default values for the Values API #350

Conversation

marcoroth commented Dec 12, 2020 • edited Loading

Example

Override custom default values

leastbad commented Dec 12, 2020

dancallaghan commented Dec 12, 2020

javan commented Dec 14, 2020 • edited Loading

jaredcwhite commented Dec 14, 2020

leastbad commented Dec 14, 2020

dancallaghan commented Dec 14, 2020 • edited Loading

paulozoom commented Dec 14, 2020 • edited Loading

julianrubisch commented Dec 16, 2020

DamnedScholar commented Dec 16, 2020

dancallaghan commented Dec 16, 2020

leastbad commented Dec 16, 2020

adrienpoly commented Dec 17, 2020

darylbarnes commented Dec 17, 2020 via email

cabgfx commented Dec 17, 2020

inopinatus commented Jan 2, 2021

leehericks commented Mar 30, 2021 • edited Loading

leehericks commented Mar 30, 2021 • edited Loading

marcoroth commented Mar 31, 2021

dhh commented Jun 13, 2021 • edited Loading

marcoroth commented Dec 12, 2020 •

edited

Loading

javan commented Dec 14, 2020 •

edited

Loading

dancallaghan commented Dec 14, 2020 •

edited

Loading

paulozoom commented Dec 14, 2020 •

edited

Loading

leehericks commented Mar 30, 2021 •

edited

Loading

leehericks commented Mar 30, 2021 •

edited

Loading

dhh commented Jun 13, 2021 •

edited

Loading