[RFC] Refactor shared logic into withMethod and withProperty helpers #22
Conversation
| @@ -33,7 +33,41 @@ function isjQuery(node) { | |||
| return id && id.name.startsWith('$') | |||
| } | |||
|
|
|||
| function withProperty(property, callback) { | |||
There was a problem hiding this comment.
This helper function encapsulates the visitor logic when checking a property on the jQuery global, for example:
$.get()I'm not convinced on the name but this with the best thing I could come up with at the time. 😂
| } | ||
| } | ||
|
|
||
| function withMethod(method, callback) { |
There was a problem hiding this comment.
This helper function encapsulates the visitor logic when checking a method call on a jQuery object, for example:
// the .each()
$('li').each()Again, open to clearer naming if you have suggestions.
| if (node.callee.object.name !== '$') return | ||
| if (node.callee.property.name !== 'globalEval') return | ||
|
|
||
| CallExpression: utils.withProperty('globalEval', function(node) { |
There was a problem hiding this comment.
The callback function is called when a match is found, which allows the caller to call context.report() and/or access the node itself.
| message: 'Prefer fetch to $.' + name | ||
| }) | ||
| CallExpression: utils.withProperty( | ||
| ['ajax', 'get', 'getJSON', 'getScript', 'post'], |
There was a problem hiding this comment.
You can also pass an array of property or method names into these util functions if you want to check multiple property/method names.
|
|
||
| module.exports = function(context) { | ||
| const enforce = utils.withProperty('Deferred', function(node) { |
There was a problem hiding this comment.
Since these helper functions return a function, you can assign it to a variable and use it as a visitor callback for multiple node types (like the CallExpression and NewExpression in this example).
|
|
||
| return function(node) { | ||
| if (node.callee.type !== 'MemberExpression') return | ||
| if (node.callee.object.name !== '$') return |
There was a problem hiding this comment.
In regards to #18, if we pass the ESLint context into the withProperty() or withMethod() helpers, we can refactor this line to support jQuery global aliases:
// pseudocode
const aliases = getjQueryAliases(context)
if (!aliases.has(node.callee.object.name)) return
|
What's the status of this PR? |
Thinking about supporting the request in #18, it would currently require a lot of similar changes to each rule to support different jQuery aliases.
What are your thoughts on a refactor to include a couple of util functions to encapsulate shared business logic so that it's easier to add new rules and also make changes like the request in #18 with fewer code changes down the line?
I know this plugin is in a pretty stable state and supports a lot of jQuery cases already, so I would understand if you are not interested in sweeping changes, but thought I would open a WIP PR to highlight these APIs (comments inline) and see what your thoughts are. Thanks!