search

ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.12k stars 13.5k forks source link

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

Closed ebk46 closed 2 years ago

ebk46 commented 2 years ago

Prerequisites

Ionic Framework Version

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 commented 2 years ago

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 commented 2 years ago

I agree that the doubled ionChange events are a duplicate of https://github.com/ionic-team/ionic-framework/issues/20106 (the underlying behavior is the same), but we can use this issue to track the bug with the 1,2 string value.

adamduren commented 2 years ago

@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.

image
adamduren commented 2 years ago

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

https://user-images.githubusercontent.com/581097/152408838-4a05aa72-e4ec-450d-b17b-1374c03c63fe.mov

adamduren commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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:

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 commented 2 years ago

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

adamduren commented 2 years ago

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

adamduren commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/facebook/react/issues/11347.

adamduren commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

sean-perkins commented 2 years ago

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 commented 2 years ago

Thanks @sean-perkins !

captainR0bbo commented 2 years ago

@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:

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 commented 2 years ago

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: https://github.com/ionic-team/ionic-framework/discussions/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 commented 2 years ago

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[bot] commented 2 years ago

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.