-
Notifications
You must be signed in to change notification settings - Fork 304
@ionicPage module resolution issue with component file naming #1093
Comments
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. |
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). |
The suffix is configurable. It defaults to I'm not sure if it's enough, though, 'cause it'd look for Thanks, |
@wbhob Yes I know, but it doesn't have to be or at least should not be forced to. @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 |
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, |
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? |
@jakiette I have not yet started working on it, so thank you for taking the time to do it 😄
@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 |
@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, |
@danbucholtz Sure, here is a complete example, using angular-cli for the build: https://github.com/ngx-rocket/starter-kit/tree/mobile/ionic-routing As for the Angular router docs, I find it way clearer than the docs covering I can understand that you want to keep Ionic core not Angular-dependant, though what ticks me is the |
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 I opted to rename each component file and remove 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. |
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? |
@jakiette @Ashirogi-Muto running into the same issue, did you guys figure out a solution yet? |
@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. |
@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!). As a side note, I am curious to see how features tight to Angular/TypeScript like |
@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 😄 @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. |
Short description of the problem:
When naming my page components following the Angular Style Guide conventions, for example
home.page.ts
(orhome.component.ts
) and use@ionicPage
decorator, I get an error: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
orhome.component.ts
should resolve tohome.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:
sidemenu
starter.src/pages/home/home.ts
tohome.page.ts
and add@ionicPages
decorator on it and fixe references to it inapp.module.ts
andapp.component.ts
src/pages/home/home.module.ts
:Which @ionic/app-scripts version are you using?
"@ionic/app-scripts": "^2.0.0",
The text was updated successfully, but these errors were encountered: