Skip to content

ng g route should also add the route to the RouteConfig #287

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

Closed
7 tasks
hansl opened this issue Mar 10, 2016 · 10 comments · Fixed by #342
Closed
7 tasks

ng g route should also add the route to the RouteConfig #287

hansl opened this issue Mar 10, 2016 · 10 comments · Fixed by #342

Comments

@hansl
Copy link
Contributor

hansl commented Mar 10, 2016

Tasklist:

  • add a angular-cli/router-config.ts to the scaffold. This is part of the project. The content of this file should have a comment at the top to say to not touch this file, and export a CliRouteConfig (open to name suggestions).
  • add a README.md to the angular-cli directory to explain that this is generated code. We will use this for other things such as providers and third party libraries.
  • import the CliRouteConfig in the default src/app/__name__.ts. The code should look something like [1].
  • ng generate route needs to add the route info to the router-config.ts. This includes importing the new route and adding it to the CliRouteConfig array.
  • ng generate route should have a flag --no-route-generation (open to suggestions) that avoid the addition of it to the CliRouteConfig array.
  • README.md documentation for this.
  • e2e tests for it.

[1]

@Component({
  selector: '<%= htmlComponentName %>-app',
  providers: [],
  templateUrl: 'app/<%= htmlComponentName %>.html',
  directives: [ROUTER_DIRECTIVES],
  pipes: []
})
@RouteConfig([

].concat(CliRouteConfig))
export class <%= jsComponentName %>App {
  defaultMeaning: number = 42;

  meaningOfLife(meaning?: number) {
    return `The meaning of life is ${meaning || this.defaultMeaning}`;
  }
}
@Brocco
Copy link
Contributor

Brocco commented Mar 10, 2016

I'm hesitant to have the configuration of routes in a separate file. Is that a convention that we want to convey via the CLI?

This solution does not scale enough to provide the ability to create routes anywhere else in the application, only at the root. There are going to be cases where internal components need to configure their own routes and that should be accomplishable via the CLI.

@filipesilva
Copy link
Contributor

I second the routes-anywhere concern voiced by @Brocco. Perhaps having child routes is fine as a future enhancement though, as top level routes cover the most common scenarios.

But I agree that we need a place for cli-generated code that is completely under our control. That location needs to be part of the project, so angular-cli/ sounds as good as any.

As an aside, we already have a config/ folder that seems to be used for ember-cli env and path location config. Perhaps we want to piggyback on it?

I do have a recommendation though: since altering/regenerating that file involves complex, error prone typescript code parsing, we should instead generate it from json based config file. Not to mention that, were a user to inadvertently alter this file, the cli would possibly mangle the file due to flimsy parsing rules.

We will also need something similar to configure app-wide providers (like the ones ng install should manage), so I strongly feel we should move to config-based file generation instead of code parsing.

Consider a angular-cli.json file, that among other configuration items, contains this:

(...)
routes: [
 {
    routePath:'/hero/...', 
    component: 'HeroRoot'.
    componentPath: './hero/hero-root.component'
  }
],
(...)

From this it is trivial to generate the needed router-config.ts, perhaps even using a blueprint:

import {HeroRoot} from './hero/hero-root.component';

let CliRouteConfig = [
  {path:'/hero/...', name: 'HeroRoot', component: HeroRoot}
];

default export CliRouteConfig;

@hansl
Copy link
Contributor Author

hansl commented Mar 10, 2016

Child routes is not a concern for this issue, it's a separate issue that we need to address when doing child routes anyway.

We cannot piggyback on config/ because it's outside the src/ directory.

We cannot use JSON for this as it is not IDE friendly, unfortunately. That was my first idea.

The other solution is changing the app.ts itself. This requires parsing TypeScript and figuring out where. @zackarychapple I can help you with that if you want to go down that route.

Alternatively, we can have the simple solution first (external file), then implement the app.ts as a phase 2 project. WDYT?

@Brocco
Copy link
Contributor

Brocco commented Mar 10, 2016

I think that directly modifying components directly is the appropriate approach here.

It hides nothing from the user and they do not need to look into a different file to see the routes that are available inside a given component.

@filipesilva
Copy link
Contributor

I think perhaps I didn't explain myself properly. My strategy was to use the json file to generate the typescript file. There would always be a real code file that the IDE could use and trace.

The only difference is that, when you needed to alter it, you'd re-generate it from the json data. This is much, much better than altering by parsing typescript.

Unless I'm missing something here, having the external file will always imply parsing typescript, since that file is a typescript file and it needs to import the routable components.

@filipesilva
Copy link
Contributor

I made a proof of concept PR to demo what I was suggesting. See #292.

Running ng new testproj gives you the following two new files:

./src/angular-cli/route-config.ts

// DO NOT EDIT THIS FILE
// IT IS AUTO GENERATED BY ANGULAR-CLI

export const CliRouteConfig = [

];

./angular-cli.json

{
  "routes": []
}

After running ng g route hero, they have the following content:

./src/angular-cli/route-config.ts

// DO NOT EDIT THIS FILE
// IT IS AUTO GENERATED BY ANGULAR-CLI
import HeroRoot from '../app/hero/hero-root.component';

export const CliRouteConfig = [
{path:'/hero/...', name: 'HeroRoot', component: HeroRoot},
];

./angular-cli.json

{
  "routes": [
    {
      "routePath": "/hero/...",
      "component": "HeroRoot",
      "componentPath": "../app/hero/hero-root.component"
    }
  ]
}

This approach has the following benefits:

  • fully blueprint based
  • doesn't need any modification of the build process
  • always has 'real' code files and thus is IDE/manual compilation safe
  • no messy TS file parsing,
  • route-config.ts is never read and always re-generated (thus there is risk of breaking the project after adding a route like what happened to @hansl once while testing ng install),
  • route config parameters kept safe and separately, easy to access during an update procedure
  • also allows deleting of routes with no TS parsing (much harder to accurately delete code than to add code)
  • easy to add parameters and operate over other routes. This is important because we will need to support all route definition parameters in this functionality, because users cannot edit it directly. For instance, setting a default route needs to change all others to not be default, and this is needed to initially populate the router outlet.

@filipesilva
Copy link
Contributor

As a consideration to seamlessly support child routes, route-config.ts could be on the same directory as the file that imports it, instead of being in the separate ./src/angular-cli/ dir.

angular-cli.json would just need to contain an additional entry for each path it manages routes in:

{
  "routes": {
    "./src/app/": [{
      "routePath": "/hero/...",
      "component": "HeroRoot",
      "componentPath": "./hero/hero-root.component"
    }],
    "./src/app/childroute": [{
      "routePath": "/villain/...",
      "component": "VillainRoot",
      "componentPath": "./villain/villain-root.component"
    }]
  }
}

This way child routes would simply have their own config. It would be trivial to implement child routes using dynamic paths as @Brocco has been implementing.

@zackarychapple
Copy link
Contributor

@filipesilva for child routes would we expect something like this? ng generate route VillanRoot for HeroRoot or maybe ng generate route VillianRoot > HeroRoot. Thoughts?

@filipesilva
Copy link
Contributor

I hadn't thought about that until you asked really!

I guess the most important bit is that it needs to be something that unambiguously describes something else as the parent. It also needs to specify the relative location (to ./src) of the parent route, since you can perfectly well have it any depth and even repeated classes.

The parent must default to our defined root component.

As per @Brocco's work on dynamic paths, it should support specifying a path for the component itself and also default to a certain folder if path isn't specified (both for the generated element and the parent). We should use the helper functions he made for this purpose.

Using the normal ember-cli command args, then I suppose the most intuitive approach is:

ng generate route app/folder1/folder2/Villain --parent=app/folder1/folder2/HeroRoot

Where the following two should be equivalent:

ng generate route Villain
ng generate route app/Villain --parent=app/app

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants