-
Notifications
You must be signed in to change notification settings - Fork 12k
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.
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
feat(@schematics/angular): Added multiselect guard implements option #13109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the delay in reviewing. Looks good; just several comments.
"type": "list", | ||
"multiselect": true, | ||
"items": [ | ||
{ "value": "CanActivate", "label": "CanActivate" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the label. The label is optional and will use the value if not present.
2e747a7
to
39a36e7
Compare
@clydin Thanks for the review! I have pushed changes, let me know if any more are necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just two small suggestions.
packages/schematics/angular/guard/files/__name@dasherize__.guard.ts
Outdated
Show resolved
Hide resolved
39a36e7
to
a319734
Compare
Can you add fixes #13400? Thanks |
08baa85
to
219a6ec
Compare
"description": "Specifies which interfaces to implement.", | ||
"uniqueItems": true, | ||
"items": { | ||
"type": "string" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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"
]
....
},
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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).
@okeefem2 Could you please fix the merge conflict? |
219a6ec
to
36f78e6
Compare
Adds support for the guard schematic to use multiselect for which interfaces the guard should implement. Fixes angular#13400
36f78e6
to
b4ee4e5
Compare
next: ActivatedRouteSnapshot, | ||
state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree { | ||
return true; | ||
} | ||
<% } %><% if (implements.includes('CanActivateChild')) { %>canActivateChild( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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';
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, following @clydin comment.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Follow up to #13031, using the multiselect option for the guard schematic to ask the user which methods they would like to implement. I purposefully left out CanDeactivate because of the need to tie in a component. This would be a good candidate for a future PR though!
Fixes #13400
Thanks,
Michael