feat: adds no-metal-plugins rule to avoid deprecated metal-* imports #61
Conversation
| DEPRECATED_MODULES.includes(node.source.value) | ||
| ) { | ||
| context.report({ | ||
| messageId: toCamelCase(`no-${node.source.value}`), |
There was a problem hiding this comment.
Maybe this is overkill? Just did it so we had matching noMetalPromise message ids... but maybe nometalpromise would do as well? 🤷♂
There was a problem hiding this comment.
Doctors say that a bit of overkill from time to time is good for your health! But yeah, you probably could have just used 'no-metal-promise' (etc) as the key. 😂
There was a problem hiding this comment.
Or even no-metal-promise 😂
|
I've force-pushed a commit to also deprecate |
There was a problem hiding this comment.
Awesome! To make this actually work you need a couple of extra things — for a full example, see the last PR of this kind that I did:
- Add the rule to the index.
- Activate the rule in the preset.
Bonus:
- If you add docs 😁 link to the rule from the top-level README.
Even though we haven't fully eliminated all the usages of metal in liferay-portal, we can probably merge this and just add some temporary suppressions as needed. (Which reminds me that when we backport liferay-npm-scripts to All The Places™, that will bring eslint-config-liferay with it, so we'll end up needing some suppressions there too.)
| DEPRECATED_MODULES.includes(node.source.value) | ||
| ) { | ||
| context.report({ | ||
| messageId: toCamelCase(`no-${node.source.value}`), |
There was a problem hiding this comment.
Doctors say that a bit of overkill from time to time is good for your health! But yeah, you probably could have just used 'no-metal-promise' (etc) as the key. 😂
|
|
||
| create(context) { | ||
| return { | ||
| ImportDeclaration(node) { |
There was a problem hiding this comment.
To make this bullet-proof, I wonder whether we should also catch require('metal-foo'). That would be CallExpression with callee of name, 'require', and arguments being a Literal of value 'metal-foo' etc.
There was a problem hiding this comment.
We've never used (nor allowed) CommonJS, so I don't think we're likely to get hits on this one...
Maybe on old tests, but I couldn't find any occurrence...
| 'liferay-portal/no-side-navigation': 'error', | ||
| 'liferay-portal/no-metal-plugins': 'error', | ||
| 'liferay-portal/no-explicit-extend': 'error', |
There was a problem hiding this comment.
oops... I'll correct this and force-push
|
Hey @wincent, I've pushed a couple of commits simplifying the I don't think we need to care about CommonJS for now as we've never really used it... Also, regarding how to apply it, I think we might want to fork |
Yeah, that could work. I guess the question is would you rather do the suppression here by forking, or in liferay-portal by adjusting the |
Indeed, doing it in I think we can cross that bridge when the moment comes ;) |
Sounds good. |
This is a naive first pass at a rule for deprecated
metal-*imports.I originally added some other checks like usage of
CancellablePromisebut I figure that after all, once we remove theimportits usages will get flagged in different ways, so there's probably no need to go any further.