ionic-team / ionic-app-scripts

App Build Scripts for Ionic Projects
http://ionicframework.com/
MIT License
609 stars 300 forks source link

@ionicPage module resolution issue with component file naming #1093

Open sinedied opened 7 years ago

sinedied commented 7 years ago

Short description of the problem:

When naming my page components following the Angular Style Guide conventions, for example home.page.ts (or home.component.ts) and use @ionicPage decorator, I get an error:

[13:00:14]  ionic-app-script task: "serve" 
[13:00:14]  Error: /Users/noda/github/ngx-starter-kit/src/app/home/home.component.ts has a @IonicPage decorator, but it 
            does not have a corresponding "NgModule" at 
            /Users/noda/github/ngx-starter-kit/src/app/home/home.component.module.ts 

I have the corresponding module, but it's named home.module.ts, so there is an issue with the name resolution mechanism.

What behavior are you expecting?

Using home.ts, home.page.ts or home.component.ts should resolve to home.module.ts. Or if it can't be done automatically by the script, add a config option pageSuffix or equivalent so we can configure the suffix we use for page components.

Steps to reproduce:

  1. Create new ionic project, for example using the sidemenu starter.
  2. Rename src/pages/home/home.ts to home.page.ts and add @ionicPages decorator on it and fixe references to it in app.module.ts and app.component.ts
  3. Add corresponding src/pages/home/home.module.ts:
    
    import { NgModule } from '@angular/core';
    import { CommonModule } from '@angular/common';
    import { IonicPageModule } from 'ionic-angular';

import { HomePage } from './home.page';

@NgModule({ imports: [ CommonModule, IonicPageModule.forChild(HomePage) ], declarations: [HomePage], entryComponents: [HomePage] }) export class HomeModule { }



**Which @ionic/app-scripts version are you using?**
`"@ionic/app-scripts": "^2.0.0",`
wbhob commented 7 years ago

Ionic development styles are different from those of Angular. I dislike it as much as you, but I'm afraid that's the way it is, and how all Ionic developers do it.

jakiette commented 6 years ago

I've run into the same problem. I've been following Angular naming conventions since starting when Ionic wasn't offering any best practices with the assumption that something built off of a framework would inherit best practices (I'm new to using opensource, my bad there). This still doesn't seem to be an intended result, seeing as it's been noted that they may consider switch naming conventions (X) or at the very minimum don't discourage following Angular's (X). This may be the majority but certainly not -all- and it isn't even a corner case of us either, given how many people using the *.page.ts naming seen around other issues.

Really just adding that config option as noted would be great for this and save those of us who used Angular naming from having to go through and rename up to 40 or so files and any references while losing our organization. I've never seen any indication from the team that this is something other than preference. If it is, it'd be nice to see that reflected as an absolute somewhere in the docs (unless I'm blind which I very well might be).

danbucholtz commented 6 years ago

The suffix is configurable. It defaults to .module.ts. I think you can set it to whatever, though.

I'm not sure if it's enough, though, 'cause it'd look for filename.component.module.ts. I'm open to a PR here as long as it won't break existing code.

Thanks, Dan

sinedied commented 6 years ago

@wbhob Yes I know, but it doesn't have to be or at least should not be forced to. Ionic apps are Angular apps by definition, it makes no sense to artificially create divergences between communities because it just makes developer life harder. I understand Ionic team have their own opinions on how apps should be developed and they recommend following that, but this should not be imposed of devs wanting to stick to Angular's best practices.

@danbucholtz Nice to know for the PR, I'll take a look if I find some time, adding a config option for it should not be too hard

danbucholtz commented 6 years ago

Ionic apps are Ionic apps. We will always be opinionated. Our documentation for the router, for example, is about 100 time less verbose because we made pragmatic decisions. We think the Angular experience for routing/deep-linking is not great.

In the future, Ionic apps will be Angular apps, React apps, Vue apps, or even Stencil apps. The experience will always be Ionic based.

Thanks, Dan

jakiette commented 6 years ago

Alright so I have made a branch in my forked repository including the changes necessary to add this config option to IonicPage (if @sinedied already has a PR in the works please feel free to use that instead as they have more experience here. I wasn't sure if I'd be able to accomplish this or if sinedied would have time, apologies for any inconvenience).

I've tried to follow the main Ionic's contributor guidelines for ease and to also keep the changes low. The only issue I've run into is that the IonicPage metadata is set in the ionic-angular module and would need a concurrent update to not throw an error (also where the documentation update would be it seems). I actually made my GitHub for this issue so I've no experience in open source things like this, what is the protocol for handling that for PRs? Or does that signal something that ought to be left to the Ionic team as a level of change? Apologies for the questions and if I missed the answers somewhere, I've tried checking around but I couldn't find anything on this.

sinedied commented 6 years ago

@jakiette I have not yet started working on it, so thank you for taking the time to do it ๐Ÿ˜„

We think the Angular experience for routing/deep-linking is not great.

@danbucholtz Could you please elaborate on that? I have a working app which tied the Angular router to Ionic's navigation and it's working great (and way cleaner than the @ionicPages auto generated deep links in my opinion). I also worked for 2 years on a Ionic v1 app with very complex navigation (100+ screens), using ui-router and a custom ionic patch to even allow navigation in modals, we did not see any limitations tied to the router (besides declaring routes) so I'm curious. The new Angular router even allows multiple parallel states (which was not possible with ui-router), so very complex navigation use cases are possible.

danbucholtz commented 6 years ago

@sinedied, can you share an example of tying to the angular router?

Configuring the Angular router is confusing and the docs are bad, that is our primary complaint with it. We also are going to be moving core functionality within Ionic away from Angular in the near future. Developers building apps can still use angular, but none of the guts of Ionic will feature any Angular.

@jakiette, sweet, submit a PR and we'll chat about it! Thank you ๐Ÿ’ฏ

Thanks, Dan

sinedied commented 6 years ago

@danbucholtz Sure, here is a complete example, using angular-cli for the build: https://github.com/ngx-rocket/starter-kit/tree/mobile/ionic-routing The basic idea is to catch angular router events, and have Ionic navigation acting accordingly. This even allows using Angular router lazy loading (though not used here) with no efforts.

As for the Angular router docs, I find it way clearer than the docs covering @IonicPages and deep linking, but it's debatable ๐Ÿ˜‰

I can understand that you want to keep Ionic core not Angular-dependant, though what ticks me is the @IonicPages decorator: why do you need to parse the metadata it generates using a custom tool at build time to generate more code and inject it, instead of using the directly the decorator to to the same logic at run time which would be way simpler (and more compatible if you want to expand to other frameworks later)? Or maybe I missed an important point here?

bellizio commented 6 years ago

I just encountered this issue as well as I was creating separate ngModules and implementing lazy loading. I'm used to naming each component TS file with .component in the name as the Angular styleguide recommends and have been doing so for well over a year while developing with Angular 2+. So naturally I have followed the same pattern in our Ionic app up until now.

I opted to rename each component file and remove .component from the filename in favor of having a less verbose module filename (home.module.ts vs home.component.module.ts).

Hopefully there will be more flexibility with file naming conventions in the future with Ionic. I think it would be beneficial to both experienced Angular devs just getting started with Ionic and devs who are new to both frameworks.

Ashirogi-Muto commented 6 years ago

Hi there. I come from Angular background and now I am really confused about the naming convention. I have been using the Angular naming convention but I got an error while implementing lazy loading because of the module file name. So is there any guide I can refer to for proper naming convention in Ionic?

dagmawim commented 6 years ago

@jakiette @Ashirogi-Muto running into the same issue, did you guys figure out a solution yet?

wbhob commented 6 years ago

@dagmawim there isn't a solution. Basic explanation: Ionic is opinionated. There is a right way and a wrong way, and the only way to do it is name everything how Ionic asks you to. Yes, it may be different from Angular best practices, but it makes sense: Angular is for SPAs that typically have structure to pages and features. Ionic apps are mobile, which are less structured, more free-form, and need to be flexible to go from any one page to another.

sinedied commented 6 years ago

@wbhob sure Ionic is opinionated, but I disagree on your explanation: Ionic apps are not only mobile apps but also web apps (and even more PWA), and you can use Ionic to push large-scale web & mobile apps using the app (we do!). So, there no real backing reason on this particular issue beside that the Ionic team chose simplicity over Angular's best practice on this matter, but there's the @jakiette PR to fix that and everyone can be happy then ๐Ÿ™‚

As a side note, I am curious to see how features tight to Angular/TypeScript like @IonicPage will become after the next Ionic@4 version is released, that will allow to use Ionic components in any framework and not only Angular.

jakiette commented 6 years ago

@wbhob While there currently may not be a solution, this is an open issue that's marked as an enhancement and was said to accept PR for this issue. While no one on the Ionic team has touched this or the PR lately (they have barely touched this repo's issues at all in the past month, I'm assuming due to all the work they have and cool stuff they've been working on!), it's still a consideration that's being considered from last team word, so I'd wait on that before decrying the right wrong and only way fully just yet.

The naming conventions don't go anti flexibility in my opinion, they make it easier to work with different things and keep reading what's what allowing easier flexibility for me at least - if anything just adding an option should greatly increase flexibility so I'm confused on that. Sinedad beat me to the rest of it but I agree with them. End of the day it's up to the team though of course.

I'm also very interested to see how v4 will work with everything. Looking forward to it very much - even though I've started off Angular it'll be cool to see and probably try out other frameworks that fit in with Ionic. Variety with development and I'm sure more developers with different methods should provide lots more to choose the best method for the project and it'll be very nice :smile:

@dagmawim I've put off dealing with lazy loading until this is resolved as well, so current solution is to either rename everything and possibly waste time or just wait and see what the word is from the team when they have time for this and adjust it then. Depends on your schedule I'd think.