Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: componentProps of modalController does not work with Angular 17.1.0 signal inputs #28876

Closed
3 tasks done
ntorrey opened this issue Jan 25, 2024 · 10 comments
Closed
3 tasks done
Labels
package: angular @ionic/angular package type: bug a confirmed bug report

Comments

@ntorrey
Copy link

ntorrey commented Jan 25, 2024

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

Passing componentProps (when using a modalController) throws an error when the input of the component is a signal (Input signals are a feature of Angular 17.1.0).

Expected Behavior

The componentProps should work even if the component input is a signal.

Steps to Reproduce

  1. Create a component to pass to a modalController which has signal inputs.
  2. Create a modal controller with componentProps that correspond to the component's input signals.
  3. Open the modal and observe the results with dismay.

Code Reproduction URL

https://github.com/ntorrey/ionbug

Ionic Info

Ionic:
Ionic CLI : 7.2.0
Ionic Framework : @ionic/angular 7.6.6
@angular-devkit/build-angular : 17.1.1
@angular-devkit/schematics : 17.1.1
@angular/cli : 17.1.1
@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 (C:\Program Files\nodejs\node.exe)
npm : 10.4.0
OS : Windows 10

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jan 25, 2024
@liamdebeasi liamdebeasi self-assigned this Jan 25, 2024
@liamdebeasi liamdebeasi added package: angular @ionic/angular package type: bug a confirmed bug report labels Jan 25, 2024
@ionitron-bot ionitron-bot bot removed the triage label Jan 25, 2024
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jan 25, 2024

Thanks! Here's a dev build with a fix: 7.6.2-dev.11706205501.196a5433

Note that this is an Ionic 8 build and is subject to breaking changes.

@liamdebeasi liamdebeasi removed their assignment Jan 25, 2024
@ntorrey
Copy link
Author

ntorrey commented Jan 26, 2024

Seems to work now - thanks for the quick response!

@ntorrey ntorrey closed this as completed Jan 26, 2024
@liamdebeasi
Copy link
Contributor

Thanks for testing! I'm going to keep this open until the linked PR is merged.

@EinfachHans
Copy link
Contributor

Just mentioned it has not been included in v8?

@liamdebeasi
Copy link
Contributor

No, the PR has not been merged.

@SadraRahmani

This comment was marked as off-topic.

sean-perkins added a commit that referenced this issue May 6, 2024
Issue number: resolves #28876

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When assigning `componentProps` as inputs to an Angular component, we do
`Object.assign`. When using the newer Angular Signals API for inputs the
value of an input is a function:

```js
myInput = input<string>('foo') // this is a function
```

The developer accesses the value of `myInput` in a template by doing
`myInput()` since `myInput` is a function.

If a developer passes `componentProps: { myInput: 'bar' }` then the
value of `myInput` is set to this string value, overriding the function.
As a result, calling `myInput()` results in an error because `myInput`
is a string not a function.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Angular 14.1 introduced `setInput` which lets us hand off setting
inputs to Angular. This will set input values properly even when using a
Signals-based input.

## Does this introduce a breaking change?

- [x] Yes
- [ ] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


As part of this `NavParams` has been deprecated as it is incompatible
with the `setInput` API. The old `Object.assign` worked to allow devs to
get all of the `componentProp` key value pairs via `NavParams` even if
they are not defined as `Inputs`. Using `setInput` will now throw an
error, so developers need to create an `@Input` for each parameter. This
means that `NavParams` has no purpose and can safely be retired in favor
of Angular's Input API. Not removing NavParms would make it difficult
for us to support new Angular APIs such as this Signals-based input API.

## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev build: `8.1.1-dev.11715021973.16675b67`

You will need to update the Ionic config to opt-in to the new option:
```ts
useSetInputAPI: true,
```

---------

Co-authored-by: Liam DeBeasi <[email protected]>
@ntorrey
Copy link
Author

ntorrey commented May 9, 2024

Woohoo! Can't wait for the 8.2 release. It will be good to finally move off of the dev version I've been using (not in production of course 😬). Thanks for your work on this!

@sean-perkins
Copy link
Contributor

Thanks for reporting this issue! This has been resolved with #29453 and will be available in the next minor release 🎉

@sean-perkins
Copy link
Contributor

sean-perkins commented May 22, 2024

👋 this is now available in v8.2.0. Upgrade your apps and let us know if you experience any issues!

Edit:

This feature is opt-in, so be sure to update your Ionic config to include:

useSetInputAPI: true,

@muuvmuuv
Copy link

Works! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants