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.09k stars 13.51k forks source link

bug: (Angular 17.2) Model Inputs break modal and popover controllers. #29013

Closed ntorrey closed 9 months ago

ntorrey commented 9 months ago

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

Similar to https://github.com/ionic-team/ionic-framework/issues/28876, with the new model signal APIs soon-to-be introduced in Angular 17.2, the modalController and popoverController do not set the model inputs correctly. Not sure if you want to tackle this preemptively since it's a bug based on a non-stable release of Angular. (Edit: The stable version was released. I updated the reproduction.)

Expected Behavior

I expect the model inputs to be set correctly without any errors.

Steps to Reproduce

  1. Display a component with a model input via the popover or modal controllers, and set the model property in the componentProps of the controller. (don't confuse model with modal 😄)

Code Reproduction URL

https://github.com/ntorrey/model-input-bug

Ionic Info

Ionic: Ionic CLI : 7.2.0 Ionic Framework : @ionic/angular 7.6.2-dev.11706205501.196a5433 @angular-devkit/build-angular : 17.2.0 @angular-devkit/schematics : 17.2.1 @angular/cli : 17.2.0 @ionic/angular-toolkit : 11.0.0

Capacitor:

Capacitor CLI : not installed @capacitor/android : not installed @capacitor/core : not installed @capacitor/ios : not installed

Utility:

cordova-res : not installed globally native-run : not installed globally

System: NodeJS : v18.18.0 npm : 10.4.0 OS : Windows 10

Additional Information

I'm using the dev build (7.6.2-dev.11706205501.196a5433) that was created in https://github.com/ionic-team/ionic-framework/issues/28876.

liamdebeasi commented 9 months ago

Thanks for the report. I'm going to merge this with https://github.com/ionic-team/ionic-framework/issues/28876. The underlying issue is the same, but it looks like my setInput fix in the linked PR isn't enough to resolve the issue. I'll see if there's a better way to resolve this and post updates in the linked thread.

liamdebeasi commented 9 months ago

Hey there! I spoke with the team, and the usage in your demo is incorrect which results in the error. Since myModelInput is already a writeable signal, you don't need to pass another signal when setting componentProps, just the value of the signal is sufficient.

For example, your application passes signal(['one', 'two', 'three']), but you only need to pass ['one', 'two', 'three']. Under the hood we use Angular's setInput API which will take care of correctly updating the signal value while still keeping the data reactive.

ntorrey commented 9 months ago

@liamdebeasi Interesting! It works, but only partially. First of all, that's counter-intuitive to what I originally thought since in an angular component, if you want bi-directional reactivity with a model input you have to pass the signal itself and not just the 'unwrapped' value (by calling it with a () at the end.) Secondly, after testing I noticed that only the first value is pushed back to the original signal in the app.component. I updated the reproduction which hopefully gets the point across.

ntorrey commented 8 months ago

@liamdebeasi Sorry for the ping! Just wanted to make sure my comment above doesn't get lost in the shuffle since this issue is marked as closed. Should it be reopened?

liamdebeasi commented 8 months ago

It's possible that we need more clarification from the Angular team, but according to the existing documentation the usage noted in https://github.com/ionic-team/ionic-framework/issues/29013#issuecomment-1946592837 is correct.

It does not seem that the controllers have ever supported two way bindings via a single property, so that could be part of the issue here. The controller approach isn't ideal for reactive properties which is why we introduced the inline overlay syntax. You might want to try using an inline modal here instead.

ntorrey commented 8 months ago

Being able to use two-way binding via a controller would be great DX for me, but I get how it is complicated. The inline overlay syntax isn't as handy for modal components that I want to reuse in many different places.

ionitron-bot[bot] commented 7 months 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.