Add catchRouterMain option (default false) to no-private-routing-service rule
#871
Conversation
| @@ -6,6 +6,7 @@ Disallow the use of: | |||
|
|
|||
| * The private `-routing` service | |||
| * The private `_routerMicrolib` property (when the `catchRouterMicrolib` option is enabled) | |||
| * The private `router:main` property (when the `catchRouterMain` option is enabled) | |||
There was a problem hiding this comment.
Are there any plans to rename this to _router:main or something like that, to make it more obvious that it shouldn't be used?
There was a problem hiding this comment.
No, I think it will just be removed eventually, and until that happens, it's the job of linting to make it clear that it shouldn't be used.
| @@ -93,6 +104,24 @@ module.exports = { | |||
| context.report({ node, message: ROUTER_MICROLIB_ERROR_MESSAGE }); | |||
| } | |||
| }, | |||
|
|
|||
| CallExpression(node) { | |||
There was a problem hiding this comment.
Should we also check OptionalCallExpression for owner?.lookup('router:main')?
There was a problem hiding this comment.
I guess so! I updated to catch that. Of course, it would be pretty bad practice to use optional chaining since lookup should always exist on owner.
There was a problem hiding this comment.
This example would be the scenario where owner is undefined. For that, we'd need to handle owner.lookup?.('router:main')
| }, | ||
| { | ||
| // Optional chaining. | ||
| code: "this?.owner?.lookup('router:main');", |
There was a problem hiding this comment.
Could you do lookup?.( here?
| // x?.lookup('router:main') | ||
| if ( | ||
| catchRouterMain && | ||
| (types.isMemberExpression(node.callee) || types.isOptionalMemberExpression(node.callee)) && |
There was a problem hiding this comment.
It could make sense to make an isOptionalMemberOrMemberExpression function.
There was a problem hiding this comment.
That makes sense given we'll probably need to use this combination in many places...
There was a problem hiding this comment.
It could almost make sense to rename isMemberExpression to isNonoptionalMemberExpression, to remind people that they should also consider the optional version. Though that could stray to far from the simple is... methods.
| node.callee.property.name === 'lookup' && | ||
| node.arguments.length === 1 && | ||
| types.isLiteral(node.arguments[0]) && | ||
| typeof node.arguments[0].value === 'string' && |
There was a problem hiding this comment.
This check isn't actually necessary, since you're also checking its value with ===.
bmish
changed the title
catchRouterMain option to no-private-routing-service rulecatchRouterMain option (default false) to no-private-routing-service rule
Disallows this in favor of using the public routing service:
Fixes #762.
CC: @mongoose700 @mehulkar