Calcite-switch won't switch if not wrapped in <label>

[vcolavin]

cc @paulcpederson @patrickarlt @macandcheese
I will include a fix for this in my upcoming PR for #1.

The most basic usage of calcite-switch is broken, probably as a result of PR 48 (which I wrote):

<calcite-switch></calcite-switch>

You would expect that clicking on the resulting switch would toggle the value, but it doesn't. It works when wrapped in a label, like this:

<label>
  <calcite-switch></calcite-switch>
  click here!
</label>

This is happening because the component's switched prop gets immediately toggled back by the proxyInput mutation observer.

The proximal cause of this bug is in the mutation observer:

this.switched = this.inputProxy.checked; // returns empty string or null
// should be this instead:
this.switched = this.inputProxy.hasAttribute("checked");

More fundamentally however I wrote this bug as a result of a confusing dataflow graph. The DOM is the source of truth, so props are synced to it, but the DOM itself gets updated based on props sometimes:

• user clicks
• toggle this.switched prop
• trigger switchWatcher
• toggle checked attribute on inputProxy
• trigger inputProxy mutation observer
• set this.switched to inputProxy.checked

I don't have a solution for this deeper issue. Does anyone else have thoughts on this?

[vcolavin]

It would be more correct to say that there are currently two sources of truth for state: changes to props or changes to the underlying DOM are both respected and propagated to one another. This is in order to support both the wrapped and non-wrapped syntaxes:

<calcite-switch></calcite-switch>
<calcite-switch>
  <input type='checkbox' />
</calcite-switch>

One solution would be to disambiguate internally between these two fairly distinct use cases: if no checkbox is provided, treat props as the source of truth. If a checkbox is provided, treat the DOM as the source of truth.

This would allow the component to maintain a single-direction data flow, preventing a whole class of nasty bugs.

[paulcpederson]

@patrickarlt this passing in your input approach makes the story for these components kind of complicated and adds a lot to the implementation. We could move a lot faster if we removed this requirement.

Wondering if we should go out of our way for this at v1. Maybe bindings for other frameworks are a feature add in the future?

[vcolavin]

For posterity I'll post this here in addition to Teams:

https://github.com/ionic-team/ionic/tree/master/core/src/components/checkbox#angular

Ionic, which uses Stencil internally, is able to support Angular's special attribute syntax, like<ionic-component [(ngModel)]='something' />. I'm trying to understand how they're able to do this, and how @ionic/angular contributes to that. They certainly don't wrap every single component.

[vcolavin]

I think Angular and Vue can pretty much use Ionic's web components directly (with some exceptions: Vue's ionic-tabs for some reason, detailed here).

React is different. Every component has to be wrapped. Mostly this is done semi-automatically using a helper function called createComponent.

[patrickarlt]

@vcolavin this depends on what you mean by "use directly" and the level of support we want to provide for each framework. Ionic ships a set of Angular bindings so if by "use directly" you mean:

<calcite-switch (switched)="onSwitched(e)" [switched]="isEnabled"></calcite-switch>

You can actually do that without the Angular bindings because it just uses the DOM events and attributes directly.

But if by "use directly" you mean how you are SUPPOSED to do Angular forms like:

<calcite-switch [(ngModel)]="isEnabled"></calcite-switch>

Then you need the bindings. For example here are the <ion-toggle> bindings.

The reason we do things like:

<calcite-switch>
  <input type='checkbox' />
</calcite-switch>

Is discussed in #24 (comment) but to summarize:

1. We avoid having to write and maintain binding for every framework.
2. Standard browser behaviors like wrapping <calcite-switch> in a <label> work because the standard browser behavior will be to update the nested <input> which will then update the switch.

---

@paulcpederson @vcolavin this honestly just seems like a bug we need to fix and write solid regression tests for which I can do after UC.

@paulcpederson we could remove the requirement for this work as per #67 but I think that would be a mistake since it would encourage writing lots of custom additional behavior in the bindings and it would be lots of work the first team that would be implementing these i.e. I have to write Angular binding just to use these in VTSE and then every other team writes their own bindings. I suppose we could already start looking into Stencil DS which is supposed to be able to automatically write the binding for you if we want to have this discussion now. But I honestly feel like this is bug to be solved and not cause us to rethink the whole approach.

[vcolavin]

Thanks for the well-thought-out comment @patrickarlt! I agree that this is just a bug that just needs to be fixed. And maybe this isn't the time or place to relitigate the basic approach to writing calcite-components. I did come onto this project when it was already in progress. It sounds like there are serious downsides to the wrapper route.

But because this is intended to be a library which is used and maintained across Esri products for a long while, we will want to prioritize maintainability and code quality. Circular updates stand out to me as a maintainability and readability red flag. I want to find some way or pattern that makes them harder to write, in the same way that React and other modern frameworks protect against them.

[paulcpederson]

Closing as I think this was taken care of?