bug: IonSelect w/ multiple={true} yields incorrectly formatted value in onIonChange

[ebk46]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

• v4.x
• v5.x
• v6.x

Current Behavior

When a change is made in an IonSelect with multiple={true}, it triggers onIonChange 3 times with the following behavior:

Trigger 1) Values as array
Trigger 2) Values as comma-separated string
Trigger 3) Values as array

Obviously, this is very bad as it should only trigger once, and the 2nd trigger violates the Array exected type.

Expected Behavior

onIonChange should only trigger once, and should include only an array of values, not a comma-separated string.

Steps to Reproduce

This happens with any IonSelect with multiple={true}. See example project below.

Code Reproduction URL

https://stackblitz.com/edit/react-ne3qq8

Ionic Info

Ionic:

Ionic CLI : 6.18.1 (/Users/evan/.config/yarn/global/node_modules/@ionic/cli)
Ionic Framework : @ionic/react 6.0.2

Capacitor:

Capacitor CLI : 3.3.4
@capacitor/android : 3.3.4
@capacitor/core : 3.3.4
@capacitor/ios : 3.3.4

Utility:

cordova-res (update available: 0.15.4) : 0.15.3
native-run : 1.5.0

System:

NodeJS : v16.13.2 (/usr/local/bin/node)
npm : 8.1.2
OS : macOS Catalina

Additional Information

No response

[sean-perkins]

Thanks for reporting this issue! I'm seeing the same problems within your reproduction.

Similar issues have been marked as a duplicate of #20106, but I will confirm with the team on Tuesday (Monday is a holiday).

I'm currently doing internal exploration to rewrite how form controls behave and can add this test case to the list to see if it's resolved with the change to how we notify value change events.

Regardless of the duplicate emission, the 1,2 string value is absolutely wrong and should never be emitted.

[averyjohnston]

I agree that the doubled ionChange events are a duplicate of #20106 (the underlying behavior is the same), but we can use this issue to track the bug with the 1,2 string value.

[adamduren]

@sean-perkins I have observed this behavior as well. As far as I can tell the reason the array is getting stringified is because react is detecting ion-select as a customer component and when it detects this it converts the attributes to a string.

[adamduren]

@sean-perkins following up with additional data. I updated the reproduction of #24687 to also demonstrate this bug. Video attached

ionic-select-object-stringify-bug.mov

[adamduren]

In my case it seems like setting the value in ionOnChange via the setter of useState triggers React updates ion-select using node.setAttribute(__attributeName, '' + value) which is why it gets stringified. Stencil sees this change reflected which triggers another ionOnChange.

[adamduren]

@sean-perkins do you have any idea where the root of this issue might lie? If I have the bandwidth I'd be happy to help but afraid I would need more context to do so?

The first question I have is should React see IonSelect as a Web Component? From what I understand natively IonSelect is a Web Component generated by Stencil but is exposed to React as a web component wrapped in a React element. However if that were the case I would not expect React to be calling setAttribute on the element and the call stack seems to suggest it knows it's a Web Component.

Without knowing a great deal about how React interacts with Ionic Stencil components my two guesses would be in either the creation of the wrapped element or in Stencil itself. Any thoughts would be appreciated.

[sean-perkins]

@adamduren appreciate the willingness to help out here!

All Stencil web components are wrapped in React's createElement function, so they are consumed as React components. There's some logic in the middle layer between React and our web component, that likely contributes to this issue.

I would suspect the issue is somewhere in here:

• https://github.com/ionic-team/ionic-framework/blob/main/packages/react/src/components/react-component-lib/createComponent.tsx#L55
• https://github.com/ionic-team/ionic-framework/blob/main/packages/react/src/components/react-component-lib/createComponent.tsx#L74

This utility component & functions actually comes from the output targets repository: https://github.com/ionic-team/stencil-ds-output-targets/tree/master/packages/react-output-target/react-component-lib and any manual changes you make here for verification, would need to be eventually submitted through that repository.

Any time you run npm run build from packages/core, will overwrite your local changes in those directories, so I would advise building Core once with the logs you need, then make your local changes in packages/react/src/components/react-component-lib/ and validate from the React test app or a local project.

Let me know if this information is helpful or if you have any follow-up questions!

[adamduren]

@sean-perkins thank you, this is helpful. Will let you know if I have any breakthroughs.

[adamduren]

@sean-perkins any update on the suggested fix for this issue in #24763?

[adamduren]

@sean-perkins hate to be a bother but any updates here? Happy to do the work to get this across the finish line.

[sean-perkins]

@adamduren appreciate the ping, I will take a look tomorrow and compare my findings against your PR. Excluding the value from being reflected may have other impacts that I would like to explore/de-risk.

Also there's another issue with form controls and React that I have in review (#24858) that may inspire a solution if the problem ends up being with how React transforms the value.

I'll share where I'm at tomorrow afternoon. I'd love to get this bug resolved.

[sean-perkins]

After triaging this issue, Adam's assessment of this code in react-dom causing the issue is correct:

https://github.com/facebook/react/blob/72a933d2892dac9e5327678b6bd37af8d589bb22/packages/react-dom/src/client/DOMPropertyOperations.js#L210-L212

When React writes the attribute value, it stringifies our value, assigns the value and this causes our Stencil web component to detect the change and emit the new value.

It looks like React has already identified this as an issue with custom elements and when those are officially supported/enabled in React, this problem should be resolved:

https://github.com/facebook/react/blob/72a933d2892dac9e5327678b6bd37af8d589bb22/packages/react-dom/src/client/DOMPropertyOperations.js#L187-L194

This problem also continues to highlight the flaw with the ionChange/form control API design. Components should not emit ionChange events when external factors manipulate the value. Only internal state changes that mutates the value, should emit the event. This would align with browser form controls and likely eliminate this issue as well.

I tried the changes of #24763 against a local reproduction, but still continued to see that our web components were emitting the value as a string 1,2.

I did discover a solution we could take within Ionic, that would listen for the change event and overwrite changes when the type changes between change events. This would likely be a custom change applied to only ion-select with multiple and not applied to all web components.

if (this.componentEl instanceof Element && (this.props as any).onIonChange) {
  this.componentEl.addEventListener('ionChange', (ev: any) => {
    const lastValue = (this.props as any).value;
    const nextValue = ev.detail.value;

    if (typeof lastValue !== typeof nextValue) {
      queueMicrotask(() => this.setState({
        value: lastValue
      }));
      return true;
    }
    
    return false;
  });
}

I will need to discuss with the team and see if they are comfortable with this solution ☝️ (would likely need additional safe guards for null/undefined values) . I have another PR that uses an extremely similar technique to solve another issue with React and setting state on value change.

[sean-perkins]

Discussed with the team, we are going to wait until React "19" (release version is still unconfirmed from React team). Applying a patch to overwrite the value of the attribute could have unexpected repercussions vs. React just fixing this internally and no hacks needed.

You can install the experimental build/branch of React that includes custom elements support:

npm install react@experimental react-dom@experimental --save

We will track the progress on this issue against React's issue: facebook/react#11347.

[adamduren]

@sean-perkins to avoid developer confusion maybe we should add to the documentation the using multiple with non-string values is not currently supported in react.

[babycourageous]

Thanks for all this great info @adamduren , @sean-perkins

Anyone landing here looking for a workaround to prevent errors or crashes. I'm currently doing a typeof check in my code to early return from that initial string list.

ie

function handleThing(selectData) {
  if (!Array.isArray(selectData)) return

  // REST OF CODE WITH ASSURANCE WE ARE WORKING WITH AN ARRAY
}

This is especially helpful for code running in an effect - which was my use-case - and causing crashes on initial run.
Can't guarantee it's foolproof but so far is doing the job.

[sean-perkins]

@babycourageous very interesting, thanks for sharing! Maybe the temporary solution could be that simple, but handled internally for you.

For example, we could skip emitting ionChange if the ion-select has multiple enabled and the value that is to be emitted is a string.

I'll do some discovery on this later in the week.

[babycourageous]

Thanks @sean-perkins for diggin' into it some more. Lemme know if you need any more info!

[sean-perkins]

Investigated this further, here is a dev-build to experiment with: 6.1.5-dev.11651861415.15120db6.

There are two conditions that we need to check for internally:

1. Is the value not an array (due to the way that React manipulates the attribute value when assigning).
2. Does the emitted value match the previous emitted value (React is still assigning the value to the component directly, so we need to still skip/ignore the value change from our setState correcting the value; to avoid duplicate ionChange events).

Here is the updated reproduction app with the dev-build: https://stackblitz.com/edit/react-x2eaeh

Overall, I am happier with this approach. It does require an opinionated change to the web component layer, but is introducing safety around the value being emitted, which does benefit apps in scenarios where the value may be mistakenly assigned as a string instead of an array. I will discuss with the team more next week + write tests around this change to validate I am not missing any unexpected side effects.

[babycourageous]

Thanks @sean-perkins !

[captainR0bbo]

@sean-perkins, @babycourageous - this dev build did resolve my issue. There is a lot going on here that I don't fully understand. but I think this might be helpful for some:

• react-hook-form 7.33.0 and Ionic 6 creates this infinite loop using a basic Ionic multi-select.
• react-hook-form 7.12.2 and Ionic 6 did NOT have this infinite loop issue
• react-hook-form 7.33.0 and @sean-perkins dev build did NOT have this loop issue

Here is an example if the issue using latest react-hook-form
https://stackblitz.com/edit/react-vhsayj?file=src%2FApp.js

Of course there a lot of versions in between but the latest react-hook-form update is what broke my implementation.

Thanks!

[liamdebeasi]

Hi everyone,

We have an update coming in Ionic 7.0 that changes how ionChange is fired from components. To summarize, ionChange will only fire when the value property is changed as a result of a user gesture. You can read more about this revision here: #25532

The reported issue is resolved by the ionChange revisions. The work has been completed and merged into our codebase, so we are going to close this ticket out. These changes should be available in Ionic 7.0. Please see our blog or Twitter to be notified when this release is published.

I have also provided a development build of Ionic 7.0 below if anyone would like to test the changes. Please note that this is a major release of Ionic and is subject to the Ionic 7.0 Breaking Changes. Let me know if you have any questions!

Dev Build:

6.3.3-dev.11666796611.134ca337

Install Example:

npm install @ionic/angular@6.3.3-dev.11666796611.134ca337

[babycourageous]

Thanks for the update @liamdebeasi !

Looking forward to the future release, and I think we have enough options to find workarounds until then!

[ionitron-bot]

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.