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

feat: angular, support binding route params to component inputs #27476

Closed medbenmakhlouf closed 1 year ago

medbenmakhlouf commented 1 year ago

Prerequisites

Ionic Framework Version

v6.x, v7.x

Current Behavior

In Angular v16, they added a new feature to help Enables binding information from the Router state directly to the inputs of the component in Route configurations.

When I add the following code as mentioned here

I got this alert in console :

'withComponentInputBinding' feature is enabled but this application is using an outlet that may not support binding to component inputs.

Screenshot 2023-05-15 at 3 57 42 PM

Expected Behavior

I am excepting to get router info from the input tag @Input

Steps to Reproduce


import { provideRouter, Routes, withComponentInputBinding } from '@angular/router';
const routes: Routes = [....]
export const ROUTE_PROVIDERS = provideRouter(routes, withComponentInputBinding());

or 

@NgModule({
  imports: [
    RouterModule.forRoot(routes, {
      //... other features
      bindToComponentInputs: true // <-- enable this feature
    })
  ],
})
export class AppModule {}

Code Reproduction URL

Editor Stackbiltz Preview Stackbiltz

Ionic Info

Ionic:
   Ionic Framework               : @ionic/angular 7.0.6
   @angular-devkit/build-angular : 16.0.1
   @angular-devkit/schematics    : 16.0.1
   @angular/cli                  : 16.0.1
   @ionic/angular-toolkit        : 9.0.0

Additional Information

No response

ionitron-bot[bot] commented 1 year ago

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

medbenmakhlouf commented 1 year ago

@Ionitron @liamdebeasi , I have added the link to reproduce the issue.

liamdebeasi commented 1 year ago

Thanks for the reproduction. Can you clarify what you'd like to use this feature for?

medbenmakhlouf commented 1 year ago

@liamdebeasi , this will improve the development experience when we work with router that include dynamic params. in the example above, I showed the example of order list/detail and show how this nice feature can help make our code more clean. here is the snippet

const routes: Routes = [
   ....
  {
    path: 'orders',
    children: [
      {
        path: '',
        component: OrderListPage,
      },
      {
        path: ':id',
        component: OrderDetailPage,
      },
    ],
  },
];

@Component({
  selector: 'orders-detail',
  templateUrl: './order-detail.html',
})
export class OrderDetailPage {
  // before : in <= Ng15
  private route = inject(ActivatedRoute);
  id$ = this.route.params.pipe(map((params) => params['id']));

  // AFTER: in >= Ng16
  @Input() id: string;
}

So you can see the binding between router param and the @Input directive. In conclusion, you can see how much code could this feature help us to clean from our mobile apps that are based on Angular.

ionitron-bot[bot] commented 1 year ago

This issue has been labeled as community feedback wanted. This label is added to issues that we would like to hear from the community on before moving forward with any final decision on the feature request.

If the requested feature is something you would find useful for your applications, please react to the original post with 👍 (+1). If you would like to provide an additional use case for the feature, please post a comment.

The team will review this feedback and make a final decision. Any decision will be posted on this thread, but please note that we may ultimately decide not to pursue this feature.

Thank you!

jkyoutsey commented 1 year ago

This should not be "maybe we'll support this". This is going to be a majorly used Angular 16 feature by all devs because it reduces boilerplate async code in an oft-used scenario. community feedback wanted is crazy. We WANT major Angular features supported. Thank you.

jkyoutsey commented 1 year ago

FYI, you can kind of work around this with a simple one-liner as long as you don't need to handle dynamic param changes. protected param1 = inject(ActivatedRoute).snapshot.params['param1'];

sean-perkins commented 1 year ago

@jkyoutsey the community feedback label helps us in scenarios such as these 🙂 there are many things we can prioritize to work on, but not all bugs or features we focus on have the same impact. Labeling this as "community feedback wanted" helps us get direct feedback on what features you + other developers want from Ionic so we continue to prioritize impactful work.

The great news around this feature request is that Ionic's router implementation is architected the same as Angular's. This means when they introduce a feature that is largely additions, it is simpler for us to implement and validate:

Here is Angular's commit that we can use to track changes against: https://github.com/angular/angular/commit/f982a3f965995c4883780b0d48cb5d1411ebad0f

sean-perkins commented 1 year ago

Hello everyone :wave: if you want to experiment with this feature and provide feedback, I would appreciate it!

Here is a dev-build:

npm install @ionic/angular@7.1.1-dev.11687550213.1ae44c4e

To enable the feature, you will need to provide an additional option to the IonicModule.forRoot

@NgModule({
  imports: [
    IonicModule.forRoot({ 
      bindToComponentInputs: true // <-- enable this feature
    })
  ]
})
export class AppModule { }

Additional details can be found on the draft PR: https://github.com/ionic-team/ionic-framework/pull/27694

This API is subject to change, pending a review from the team on a design document.

In theory this enables the feature in Angular v14-16 if you are using Ionic's router. I would recommend only using it with v16 of Angular, as the API will eventually be removed in favor of their configurations.

medbenmakhlouf commented 1 year ago

Hello @sean-Perkins, thanks for jumping on this. I will be happy to give a try for the new dev version. One question, I know that you guys implement ion-router-outlet instead of router-outlet. Is there a limitation/way to read the config bindToComponentInputs: true, from RouterModule or provideRouter as mentioned above ? Just to be consistent with angular docs?

if there is a limitation, maybe it is better to have as provider like IonicRouteStrategy ? I just dont see that config should be part of ionic config rather it should be part of router config.

This is just a feedback, but thanks for making this happen.

sean-perkins commented 1 year ago

Hello @medbenmakhlouf thanks for the feedback!

One limitation we have is the providers and injection tokens that Angular uses for this feature are only available in v16. Within Ionic's router implementation, we effectively have to copy their provider implementation to maintain the same feature functionality back to our v14 support.

An alternative strategy we've used in the past is having the developer pass the injectable into Ionic. We did this for standalone components where developers needed to pass the EnvironmentInjector to the ion-router-outlet or ion-tabs component.

That being said, for this feature we just need to define the provider. That can be done automatically for the developer through a config option on IonicModule or explicitly through the providers array. My thoughts to the config option, is that in a future major version of Ionic Framework, developers will remove that option from IonicModule.forRoot and move it to the RouterModule.forRoot. I am open to alternate APIs that we could offer until this becomes "baked in".

medbenmakhlouf commented 1 year ago

@sean-perkins , that sounds good! I have tested the changes in different cases and works perfectly! I can't wait for this to be merged so I can start cleaning many lines of code.

muuvmuuv commented 1 year ago

Can somebody explain why you simply not just extend the Angular router-outlet used in the peerDependency version, so all features will always match the expected version? IMO this will always add more and more boilerplate to Ionic.

muuvmuuv commented 1 year ago

Tested @ionic/angular@7.1.1-dev.11687550213.1ae44c4e in our App and it works pretty well for all types; query params, resolver data, path params and direct input.

sean-perkins commented 1 year ago

Hello everyone 👋 here is an updated dev-build to test with: 7.1.3-dev.11688674325.1859587e

In this version we were able to get rid of the additional config option on IonicModule.forRoot. Simply specify the bindToComponentInputs flag on your RouterModule and you are good to go!

const routes = [];

@NgModule({
  imports: [
    RouterModule.forRoot(routes, { 
      bindToComponentInputs: true // <-- enable this feature
    })
  ]
})
export class AppRoutingModule { }

re:

Can somebody explain why you simply not just extend the Angular router-outlet used in the peerDependency version, so all features will always match the expected version? IMO this will always add more and more boilerplate to Ionic.

It is difficult to summarize how our router integration works in a concise way, but Ionic requires explicit overrides to Angular's core routing implementation to provide a performant and native-feel routing experience. It unfortunately isn't straightforward to extend their router to accomplish our desired experience - however it would be great if it was! Routing is definitely one of the most complicated aspects of Ionic Framework.

ntorrey commented 1 year ago

@sean-perkins Great! Will this also work with standalone components? ie:

bootstrapApplication(AppComponent, {
  providers: [
    ...
    provideRouter(
       routes, 
       withPreloading(PreloadAllModules), 
       withComponentInputBinding()          //  <- will this work too?
     ), 
  ]
})

Also, any idea when to expect this in an official release?

sean-perkins commented 1 year ago

@ntorrey it should, but please let me know if it doesn't work as expected. We are still in the design stage for approval from the team (which led to the API improvements to automatically detect the feature being enabled from the RouterModule).

I plan on testing standalone app usage in more detail once the design is approved from the team.

The current implementation consumes the ROUTER_CONFIGURATION injection token to check if the flag is enabled, before constructing the needed providers for this feature to work with ion-router-outlet.

Once the design is approved, this feature would be slated for the next minor release (tentatively 7.2).

muuvmuuv commented 1 year ago

@sean-perkins are there any open issues on Ionic or Angular side that the router gets improved in a way that you could better extend it?

Tested your latest dev-build with a hole standalone application, no longer works.

ntorrey commented 1 year ago

Same result here. Just did some cursory testing, and while I don't get any errors, my inputs return undefined.

sean-perkins commented 1 year ago

Thanks for the feedback!

As a temporary workaround you can assign the ROUTER_CONFIGURATION value that is currently used to detect the feature:

bootstrapApplication(AppComponent, {
  providers: [
    { provide: RouteReuseStrategy, useClass: IonicRouteStrategy },
    importProvidersFrom(IonicModule.forRoot({})),
    provideRouter(routes, withComponentInputBinding()),
    {
      provide: ROUTER_CONFIGURATION,
      useValue: {
        bindToComponentInputs: true,
      }
    },
  ],
});

I may need to consult with the Angular team on this. Ideally the withComponentInputBinding() API should be assigning the same provider value as when doing RouterModule.forRoot([], { bindToComponentInputs: true }).

ntorrey commented 1 year ago

Thank you @sean-perkins! I can confirm that this workaround works. It seems that I don't need the withComponentInputBinding() to make it work if I provide the ROUTER_CONFIGURATION as you suggest.

sean-perkins commented 1 year ago

Ok, here's an updated dev-build for standalone usage that should not require manually providing a value for ROUTER_CONFIGURATION:

npm install @ionic/angular@7.1.3-dev.11689276547.129acb40

The only requirement should be:

provideRouter(routes, withComponentInputBinding()),
medbenmakhlouf commented 1 year ago

@sean-perkins thanks man! you made it the way it should be :)

sean-perkins commented 1 year ago

This feature has been merged and will be available in the upcoming v7.2 release of @ionic/angular.

Eraldo commented 1 year ago

I enabled the feature: provideRouter(routes, withComponentInputBinding()),

However I get the following warning in the console: withComponentInputBinding' feature is enabled but this application is using an outlet that may not support binding to component inputs. And the inputs are not picked up it seems. Am I missing something? 🤷‍♂️

Version 7.2.0

sean-perkins commented 1 year ago

@Eraldo

Are you able to reproduce in an Ionic starter template?

ntorrey commented 1 year ago

@Eraldo Seems to be working for me! Maybe there's a problem with the way your routes are set up? But that wouldn't explain the warning you're getting. Also, the value is not yet available in the constructor function, but on NgOnInit().

// my routes.ts file
{
  path: 'dashboard',
  children: [
    {
      path: '',
      loadComponent: () => import('./dashboard/dashboard.page')
    },
    {
      path: ':id',
      loadComponent: () => import('./dashboard/dashboard.page')
    }
  ]
},
ionitron-bot[bot] commented 1 year 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.