diff --git a/README.md b/README.md index 67159e1142..5016ad2531 100755 --- a/README.md +++ b/README.md @@ -64,6 +64,7 @@ An article with detailed setup instructions can be found [here](https://medium.c * [no-template-lifecycle-assignments](docs/rules/no-template-lifecycle-assignments.md): Prevent deprecated template lifecycle callback assignments * [eventmap-params](docs/rules/eventmap-params.md): Force consistent event handler parameter names in event maps * [prefix-eventmap-selectors](docs/rules/prefix-eventmap-selectors.md): Convention for eventmap selectors + * [scope-dom-lookups](docs/rules/scope-dom-lookups.md): Scope DOM lookups to the template instance ## Core API * *currently no rules implemented* diff --git a/docs/rules/scope-dom-lookups.md b/docs/rules/scope-dom-lookups.md new file mode 100644 index 0000000000..89af00472b --- /dev/null +++ b/docs/rules/scope-dom-lookups.md @@ -0,0 +1,71 @@ +# Scope DOM lookups to the template instance (scope-dom-lookups) + +> It’s a bad idea to look up things directly in the DOM with jQuery’s global `$()`. It’s easy to select some element on the page that has nothing to do with the current component. Also, it limits your options on rendering outside of the main document. - [source](http://guide.meteor.com/blaze.html#scope-dom-lookups-to-instance) + + +## Rule Details + +This rule aims to ensure DOM lookups are scoped to the template instance to improve performance and to reduce accidental side-effects. + +The following patterns are considered warnings: + +```js + +Template.foo.onRendered(function () { + $('.bar').focus() +}) + +Template.foo.onRendered(function () { + const $bar = $('.bar') + // .. +}) + +Template.foo.events({ + 'click .bar': function (event, instance) { + $('.baz').focus() + } +}) + +Template.foo.helpers({ + 'bar': function () { + $('.baz').focus() + } +}) + +Template.foo.onDestroyed(function () { + $('.bar').focus() +}) + +Template.foo.onRendered(function () { + jQuery('.bar').focus() +}) + +``` + +The following patterns are not warnings: + +```js + +Template.foo.onRendered(function () { + this.$('.bar').focus() +}) + +Template.foo.onRendered(function () { + Template.instance().$('.bar').focus() +}) + +Template.foo.events({ + 'click .bar': function (event, instance) { + instance.$('.baz').focus() + } +}) + +``` + +## When Not To Use It + +Disable this rule for specific lines if something outside of the template needs to be looked up and there is no way around it. + +## Further Reading + +- http://guide.meteor.com/blaze.html#scope-dom-lookups-to-instance diff --git a/lib/index.js b/lib/index.js index 9d4c049c40..afe7e30821 100755 --- a/lib/index.js +++ b/lib/index.js @@ -7,6 +7,7 @@ import eventmapParams from './rules/eventmap-params' import prefixEventmapSelectors from './rules/prefix-eventmap-selectors' import preferSessionEquals from './rules/prefer-session-equals' import templateNamingConvention from './rules/template-names' +import scopeDomLookups from './rules/scope-dom-lookups' export const rules = { @@ -18,6 +19,7 @@ export const rules = { 'prefix-eventmap-selectors': prefixEventmapSelectors, 'prefer-session-equals': preferSessionEquals, 'template-names': templateNamingConvention, + 'scope-dom-lookups': scopeDomLookups, } export const configs = { @@ -31,6 +33,7 @@ export const configs = { 'meteor/prefix-eventmap-selectors': 0, 'meteor/prefer-session-equals': 0, 'meteor/template-names': 2, + 'meteor/scope-dom-lookups': 0, }, }, guide: { diff --git a/lib/rules/scope-dom-lookups.js b/lib/rules/scope-dom-lookups.js new file mode 100644 index 0000000000..5a1805e830 --- /dev/null +++ b/lib/rules/scope-dom-lookups.js @@ -0,0 +1,41 @@ +/** + * @fileoverview Scope DOM lookups to the template instance + * @author Dominik Ferber + * @copyright 2016 Dominik Ferber. All rights reserved. + * See LICENSE file in root directory for full license. + */ + +import getPropertyName from '../util/ast/getPropertyName' + +const jQueryNames = new Set(['$', 'jQuery']) + +const relevantTemplatePropertyNames = new Set([ + 'onRendered', + 'onDestroyed', + 'events', + 'helpers', +]) + +const isJQueryIdentifier = node => + node.type === 'Identifier' && jQueryNames.has(node.name) + +const isRelevantTemplateCallExpression = node => ( + node.type === 'CallExpression' && + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'MemberExpression' && + node.callee.object.object.type === 'Identifier' && + node.callee.object.object.name === 'Template' && + relevantTemplatePropertyNames.has(getPropertyName(node.callee.property)) +) + +const isInRelevantTemplateScope = ancestors => ancestors.some(isRelevantTemplateCallExpression) + +export default context => ({ + CallExpression: node => { + if (!isJQueryIdentifier(node.callee)) return + if (!isInRelevantTemplateScope(context.getAncestors())) return + context.report(node, 'Use scoped DOM lookup instead') + }, +}) + +export const schema = [] diff --git a/tests/lib/rules/scope-dom-lookups.js b/tests/lib/rules/scope-dom-lookups.js new file mode 100644 index 0000000000..6676b514fb --- /dev/null +++ b/tests/lib/rules/scope-dom-lookups.js @@ -0,0 +1,102 @@ +/** + * @fileoverview Scope DOM lookups to the template instance + * @author Dominik Ferber + * @copyright 2016 Dominik Ferber. All rights reserved. + * See LICENSE file in root directory for full license. + */ + +import rule from '../../../lib/rules/scope-dom-lookups' +import { RuleTester } from 'eslint' +const ruleTester = new RuleTester() + +ruleTester.run('scope-dom-lookups', rule, { + valid: [ + '$(".foo")', + 'Template.foo.xyz(function () { $(".foo"); })', + ` + Template.foo.onRendered(function () { + this.$('.bar').addClass('baz') + }) + `, + ` + Template.foo.onRendered(function () { + Template.instance().$('.bar').addClass('.baz') + }) + `, + ` + Template.foo.events({ + 'click .js-bar': function (event, instance) { + instance.$('.baz').focus() + } + }) + `, + ], + + invalid: [ + { + code: ` + Template.foo.onRendered(function () { + $('.bar').addClass('baz') + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + { + code: ` + Template.foo.events({ + 'click .js-bar': function (event, instance) { + $('.baz').focus() + } + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + { + code: ` + Template.foo.onRendered(function () { + var $bar = $('.bar') + $bar.addClass('baz') + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + { + code: ` + Template.foo.helpers({ + 'bar': function () { + $('.baz').focus() + } + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + { + code: ` + Template.foo.onDestroyed(function () { + $('.bar').addClass('baz') + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + { + code: ` + Template.foo.onRendered(function () { + jQuery('.bar').addClass('baz') + }) + `, + errors: [ + { message: 'Use scoped DOM lookup instead', type: 'CallExpression' }, + ], + }, + ], +})