angular-cli icon indicating copy to clipboard operation
angular-cli copied to clipboard

New default route config proposal

Open StephenFluin opened this issue 4 years ago • 14 comments

🚀 Feature request

Command (mark with an x)

- [x] new
- [x] generate
- [x] add

Description

Right now when you create an application with routing, it creates an AppRoutingModule. This is an extra NgModule that doesn't do anything (it doesn't change the injector config, and it isn't used for compilation of any templates).

Describe the solution you'd like

Remove it and just export the route config directly.

https://github.com/StephenFluin/new-routeconfig-proposal/commit/558c655bdc3c440d10b98fc0749c28d4f3665e24

Describe alternatives you've considered

We could also do this 100% in the AppModule, or export const ROUTECONFIG = RouterModule.forRoot(...) to get the same providers without the extra wrapper.

StephenFluin avatar Oct 02 '19 17:10 StephenFluin

This also saves 38 bytes from production ES2015 bundles!!! :astonished:

StephenFluin avatar Oct 02 '19 17:10 StephenFluin

I agree that this introduces extra complexity (and makes the chunks heavier). We should also review angular.io (especially the style guide) and revisit if we want to recommend RoutingModules as good practice.

mgechev avatar Oct 02 '19 18:10 mgechev

A review of the style guide seems to be in order. CLI is just sticking to it. It seems like having a proper review process for the style guide is where the discussion should happen and the CLI team is not in a good position to decide all of these issues.

vikerman avatar Oct 03 '19 17:10 vikerman

I'll schedule a style guide meeting to discuss further.

mgechev avatar Oct 07 '19 18:10 mgechev

I agree with this proposal. It's been discussed at some length within the GDE channel for some months now. Consensus has formed around elimination of a RoutingModule. The style guide should be updated.

Splaktar avatar Oct 08 '19 19:10 Splaktar

@StephenFluin

vikerman avatar Oct 10 '19 17:10 vikerman

+1 for this. The mental overhead of adding yet another module is not really beneficial. Much simpler with just exporting a routes array instead of a module.

mhartington avatar Nov 07 '19 18:11 mhartington

I'll do another attempt to reschedule a style guide meeting since most folks were occupied with external events.

mgechev avatar Nov 27 '19 13:11 mgechev

I’m fine either way on this. But I do have context for why this was created. I’ll share more when we meet.

Something to consider is that the routing setup gets lengthy with route definitions , imports for components , guards, etc. the adding that to a normal module can make that module file longer - and arguably conflates what it does. Meaning it’s not longer doing one thing - it is handling the ng module and routing.

Other frameworks don’t have ng module bit they do still separate routing into its own file.

One thing I’ve played with a lot of moving the routes them selves and all routing setup into its own file. Then importing that into the ng module. Still two files but you lose the extra no-op ng module.

Just some things to think about.

johnpapa avatar Dec 06 '19 01:12 johnpapa

@johnpapa I think that is what this proposal is all about. You can see an example in the OP which links here. The proposal is not to put all of the routing configuration in the AppModule.

Splaktar avatar Dec 10 '19 18:12 Splaktar

The proposal we discussed during the style guide meetings:

// routes.ts
export const routes = [
  {...},
  {...},
];
// app.module.ts
import { routes } from './routes';
...
imports: [
  ...
  RouterModule.forRoot(routes)
]

We want to keep the routes declared into a separate file called routes.ts without declaring a separate NgModule.

mgechev avatar Jan 31 '20 21:01 mgechev

Is further discussion still needed? Can this move forward?

Splaktar avatar Jan 31 '20 23:01 Splaktar

My two cents: I usually name the file app.routes.ts and not just routes.ts. This is consistent with app.component.ts and app.module.ts, and it promotes a similar naming for child modules: heroes.routes.ts, admin.routes.ts, etc., which IMHO is better than having multiple routes.ts files.

jnizet avatar May 31 '20 21:05 jnizet

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

angular-robot[bot] avatar Jun 04 '21 14:06 angular-robot[bot]

Thanks for reporting this issue. This issue is now obsolete due to changes in the recent releases. Please update to the most recent Angular CLI version.

If the problem persists after upgrading, please open a new issue, provide a simple repository reproducing the problem, and describe the difference between the expected and current behavior.

alan-agius4 avatar Dec 12 '23 13:12 alan-agius4

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.