Skip to content

feat(@schematics/angular): Added multiselect guard implements option #13109

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 15, 2019
Merged

feat(@schematics/angular): Added multiselect guard implements option #13109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/schematics/angular/guard/files/__name@dasherize__.guard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,24 @@
import { Injectable } from '@angular/core';
import { CanActivate, ActivatedRouteSnapshot, RouterStateSnapshot, UrlTree } from '@angular/router';
import { <%= implementationImports %>ActivatedRouteSnapshot, RouterStateSnapshot, UrlTree } from '@angular/router';
import { Observable } from 'rxjs';

@Injectable({
providedIn: 'root'
})
export class <%= classify(name) %>Guard implements CanActivate {
canActivate(
export class <%= classify(name) %>Guard implements <%= implementations %> {
<% if (implements.includes('CanActivate')) { %>canActivate(
next: ActivatedRouteSnapshot,
state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {
return true;
}
<% } %><% if (implements.includes('CanActivateChild')) { %>canActivateChild(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can eventually use the enum type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alan-agius4 Are you saying to create an enum for the values instead of comparing hard coded strings? If yes would this live in the index.ts file or should it have it’s own file?

Copy link
Collaborator

@alan-agius4 alan-agius4 Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, the enum is created from the json schema, you don't need to redefine it.

You can run yarn build it will generate an new schema with the enum you created in the json schema.

So you can import it like such

import { Schema as GuardOptions, Implement } from './schema';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh very cool. I will make these changes later today when I get home. Thanks!

next: ActivatedRouteSnapshot,
state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {
return true;
}
<% } %><% if (implements.includes('CanLoad')) { %>canLoad(
route: Route,
segments: UrlSegment[]): Observable<boolean> | Promise<boolean> | boolean {
return true;
}<% } %>
}
16 changes: 16 additions & 0 deletions packages/schematics/angular/guard/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,20 @@ export default function (options: GuardOptions): Rule {
if (options.path === undefined) {
options.path = buildDefaultPath(project);
}
if (options.implements === undefined) {
options.implements = [];
}

let implementations = '';
let implementationImports = '';
if (options.implements.length > 0) {
implementations = options.implements.join(', ');
implementationImports = `${implementations}, `;
// As long as we aren't in IE... ;)
if (options.implements.includes('CanLoad')) {
implementationImports = `${implementationImports}Route, UrlSegment, `;
}
}

const parsedPath = parseName(options.path, options.name);
options.name = parsedPath.name;
Expand All @@ -47,6 +61,8 @@ export default function (options: GuardOptions): Rule {
const templateSource = apply(url('./files'), [
options.skipTests ? filter(path => !path.endsWith('.spec.ts')) : noop(),
template({
implementations,
implementationImports,
...strings,
...options,
}),
Expand Down
27 changes: 26 additions & 1 deletion packages/schematics/angular/guard/index_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { Schema as ApplicationOptions } from '../application/schema';
import { Schema as WorkspaceOptions } from '../workspace/schema';
import { Schema as GuardOptions } from './schema';


describe('Guard Schematic', () => {
const schematicRunner = new SchematicTestRunner(
'@schematics/angular',
Expand Down Expand Up @@ -64,4 +63,30 @@ describe('Guard Schematic', () => {
appTree = schematicRunner.runSchematic('guard', defaultOptions, appTree);
expect(appTree.files).toContain('/projects/bar/custom/app/foo.guard.ts');
});

it('should respect the implements value', () => {
const options = { ...defaultOptions, implements: ['CanActivate']};
const tree = schematicRunner.runSchematic('guard', options, appTree);
const fileString = tree.readContent('/projects/bar/src/app/foo.guard.ts');
expect(fileString).toContain('CanActivate');
expect(fileString).toContain('canActivate');
expect(fileString).not.toContain('CanActivateChild');
expect(fileString).not.toContain('canActivateChild');
expect(fileString).not.toContain('CanLoad');
expect(fileString).not.toContain('canLoad');
});

it('should respect the implements values', () => {
const implementationOptions = ['CanActivate', 'CanLoad', 'CanActivateChild'];
const options = { ...defaultOptions, implements: implementationOptions};
const tree = schematicRunner.runSchematic('guard', options, appTree);
const fileString = tree.readContent('/projects/bar/src/app/foo.guard.ts');

// Should contain all implementations
implementationOptions.forEach((implementation: string) => {
expect(fileString).toContain(implementation);
const functionName = `${implementation.charAt(0).toLowerCase()}${implementation.slice(1)}`;
expect(fileString).toContain(functionName);
});
});
});
18 changes: 18 additions & 0 deletions packages/schematics/angular/guard/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,24 @@
"type": "boolean",
"default": false,
"description": "When true, applies lint fixes after generating the guard."
},
"implements": {
"type": "array",
"description": "Specifies which interfaces to implement.",
"uniqueItems": true,
"items": {
"type": "string"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an enum instead of general string.

Copy link
Contributor Author

@okeefem2 okeefem2 Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hansl If I change this to enum, I get the following build error:

Error: Invalid type enum in JSON Schema at Schema#/properties/implements/items

I realized that the way this is being used the items could be updated to just a list of strings, thank you for pointing that out. Let me know what you think!

Copy link
Collaborator

@alan-agius4 alan-agius4 Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be something like;

    "implements": {
      "type": "array",
      "description": "Specifies which interfaces to implement.",
      "default": "CanActivate",
      "enum": [
           "CanActivate",
           "CanActivateChild",
           "CanLoad"
      ]
      ....
    },

Copy link
Contributor Author

@okeefem2 okeefem2 Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alan-agius4 I'm sorry I could be totally missing something (my first time working with json schema 😬), but the x prompt logic requires this array to be named as items: [] as seen in https://github.com/angular/angular-cli/blob/master/packages/angular/cli/models/schematic-command.ts#L311

Are you saying in addition to the x-prompt defintion, also include the enum definition you describe here?

Thanks for your help btw! Just trying to learn and make sure I understand.

Copy link
Collaborator

@alan-agius4 alan-agius4 Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I think it should be something like;

 "implements": {
      "type": "array",
      "description": "Specifies which interfaces to implement.",
      "default": "CanActivate",
      "items": {
        "enum": [
          "CanActivate",
          "CanActivateChild",
          "CanLoad"
        ]
      },
      "x-prompt": {
        "message": "Which interfaces would you like to implement?",
        "type": "list",
        "multiselect": true,
        "items": [
          "CanActivate",
          "CanActivateChild",
          "CanLoad"
        ]
      }
}

The .... above meant, continuation with x-prompt definition.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: updated the above is I spotted a mistake

Copy link
Member

@clydin clydin Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note also that I have a followup planned to add auto-configuration of multi-select based on the schema. It will eliminate the need to manually specify most of the prompt options and is the reason I originally suggested just using a string constraint for now (this schema would be adjusted in the process either way).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, it LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin would I be able to help out with the implementation of the multi select auto configuration?? I’d love to keep learning and contributing and this could transition well from the work I’ve done so far. Let me know what you think and thanks everyone for the help so far!

Copy link
Contributor

@cyrilletuzi cyrilletuzi Feb 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin @okeefem2 @alan-agius4 For info, the decision made here causes issues like this one: cyrilletuzi/vscode-angular-schematics#32. The x-prompts is a specific use of the schema, the classic schema properties should be respected for other tools to work (ie. possible values must be in anyOf - or similar, not sure of the good property for this case). For the same reason, implements should be flagged as required (or else it should generate a correct file without it, currently ng g guard without implements generates a file with errors).

},
"x-prompt": {
"message": "Which interfaces would you like to implement?",
"type": "list",
"multiselect": true,
"items": [
"CanActivate",
"CanActivateChild",
"CanLoad"
]
}
}
},
"required": [
Expand Down