observablehq / inputs

Better input elements
https://observablehq.com/framework/lib/inputs
ISC License
125 stars 34 forks source link

Circular definition error when one part of an Inputs form refers to another #230

Closed jimjam-slam closed 2 years ago

jimjam-slam commented 2 years ago

For me, an Inputs form seems like the natural choice for a "Choose an option or type something else pattern" (although perhaps passing a datalist to a text field is a better way to go!).

But a form like this example, where the free-form text field is conditionally disabled depending on the choice made in the dropdown menu, doesn't work because of the circular definition (which makes sense, since fruitForm is the value being reacted to in OJS a far as I understand it, not fruitForm.choice).

fruitForm = Inputs.form(
  {
    choice: Inputs.select(
      ["Apple", "Banana", "Orange", "Kiwi fruit", "Other"],
      { label: "Choose your fruit" }),
    other: Inputs.text(
      {
        label: "Other",
        placeholder: "Name a less awesome fruit",
        disabled: () => fruitForm.choice != "Other"
      })
  })

Is the answer here (in terms of preferred patterns) to go with a text field and the datalist, or to separate the two controls rather than using a form to wrap them?

jimjam-slam commented 2 years ago

Now that I'm testing a solution that keeps the controls separate, I'm realising that my control is staying disabled regardless of the predicate value. Does the disabled property support reactivity? I wasn't too sure whether this comment in issue #32 referred to that (I'm still learning a bit, sorry!).

It should probably support mutation, too, like we do for value. Hmm. On second thought, supporting mutation for all properties feels like overkill, and a lot of work for questionable value.

mythmon commented 2 years ago

@jimjam-slam Could you share the notebook you're having trouble with? I'm able to disable inputs based on other inputs here: https://observablehq.com/d/5fe3c6b7efb237dd

One important thing that I think you may have missed, you need to use the viewof operator, like viewof fruitForm = ... or viewof choice = Inputs.select(...). Without that, the value of the cell is the HTML element itself, not the value you chose in the input.

The comment you quoted about mutation is about some third cell writing into the input form, not about reactive updates of dependent cells.

jimjam-slam commented 2 years ago

I'm always forgetting the viewof 😅 Thanks!

But yes, same behaviour when the inputs are combined into a form, as above or in https://observablehq.com/d/76ccecfddcece229.

If I use the predicate () => fruitForm.choice !== "Other", I get a Circular definition error. If I use () => this.choice !== "Other", the form renders but is disabled regardless of choice.

if I separate the inputs, as in your example, @mythmon, I can have one disable based on the other 😊

mythmon commented 2 years ago

Also, just to confirm, you're 100% right when you say "fruitForm is the value being reacted to in OJS [...], not fruitForm.choice". Cells are the unit of reactivity, not deep paths like fruitForm.choice.

The reason this.choice didn't work (which is clever, by the way), is because this isn't a reactive value. When the cell first evaluates, this.choice`` won't be set toOther. As the user types into the field, the JS of the cell doesn't re-evaluate, so that function doesn't get to run again to see the change. Ifthis` was a reactive value, the cell would be at risk of getting stuck in an infinite loop of constantly changing and retriggering itself. You can't partially evaluate a cell. At best it would lose state, and at worse it would lock up the page.

This isn't really an issue with Inputs.form, or with Inputs at large. It's just the way Observable works. Cells can't reference themselves in a reactive way, even if the operation seems logical.

mbostock commented 2 years ago

Closing as a discussion rather than an inputs bug/feature request. Feel free to continue discussing.

jimjam-slam commented 2 years ago

That makes senses! Thanks for confirming my thinking 😊