New rule: no-global-jquery #103
Conversation
package.json
Outdated
| @@ -50,7 +50,8 @@ | |||
| "eslint": "^3.15.0", | |||
| "eslint-config-airbnb-base": "^11.1.0", | |||
| "eslint-plugin-import": "^2.2.0", | |||
| "jest": "^20.0.4" | |||
| "jest": "^20.0.4", | |||
| "jest-environment-node-debug": "^2.0.0" | |||
There was a problem hiding this comment.
this is somewhat controversial - it's required to make the debugging of jest tests in the terminal work. However I'm perfectly happy just to install it everytime for myself - I'd welcome any thoughts on this :)
There was a problem hiding this comment.
I'm ok with this package. But then I think we should update readme and explain how to work with this + we should add an information about the workflow with ASTExplorer. That would make life of potential contributors easier. That being said, I'd prefer to move this to another PR and update Readme there as well. What do you think @jbandura ?
There was a problem hiding this comment.
+1 will move it to separate PR
lib/rules/no-global-jquery.js
Outdated
| let emberImportAliasName; | ||
| let destructuredAssignment; | ||
|
|
||
| //---------------------------------------------------------------------- |
There was a problem hiding this comment.
Let's remove those redundant comments
lib/utils/ember.js
Outdated
| */ | ||
| function getEmberImportAliasName(importDeclaration) { | ||
| if (!importDeclaration.source) return null; | ||
| if (importDeclaration.source.raw !== "'ember'") return null; |
There was a problem hiding this comment.
I think importDeclaration.source.value !== 'ember' would be enough, no need to check the raw field.
lib/utils/utils.js
Outdated
| */ | ||
| function isGlobalCallExpression(node, desctructuredName, aliases) { | ||
| const isDestructured = node && node.callee && node.callee.name === desctructuredName; | ||
| const isGlobalJquery = node.callee && aliases.indexOf(node.callee.name) > -1; |
There was a problem hiding this comment.
isGlobalJquery name is not relevant in the given context
| });`, | ||
| parserOptions, | ||
| errors: [{ | ||
| message: MESSAGE |
There was a problem hiding this comment.
If this is valid then why is there errors array here? Please check all valid test cases and remove errors if they should not be there.
There was a problem hiding this comment.
good question 😅 will fix that 👍
| * @param {Object} initialObjToBinding relevant bindings | ||
| * @return {Array} list of object pattern bindings | ||
| */ | ||
| function collectObjectPatternBindings(node, initialObjToBinding) { |
There was a problem hiding this comment.
I think we don't support one case at this moment:
const $ = Ember.$;
Are we?
There was a problem hiding this comment.
no, we're not, good catch! 👍
jbandura
force-pushed
the
53-no-global-jquery
branch
from
8ea46d8 to
d7dd7c4
Compare
|
It just occurred to me during this migration of rules from our old repo to this one - we're losing attribution of the original author during this process. We could do one of two things:
I'd prefer the first, and am happy to retrofit #99 to correctly attribute @chadhietala. Thoughts, @jbandura @michalsnik? |
|
I agree @scalvert. However, since after the first migration from ember-best-practices we cannot rely on the fact that committer == author of the rule, we'll have to add attribution to all the existing rules as well. WDYT @michalsnik? |
|
Hey @scalvert! I totally agree and I'm fine with the first solution, we
should however keep both names in those ported rules: the original author
and the commiter as he also has to adapt it accordingly, adds more tests,
improves functionality and so on. You can update #99 and I'll add credits
to other rules later.
Regarding contributors we already have that section in readme and
package.json so you can add Chad and yourself :)
Btw. After merging couple of your rules we should also change the footer of
gh-page and add linkedin logo there too.
…On Fri, 21 Jul 2017 at 19:13, Jacek Bandura ***@***.***> wrote:
I agree @scalvert <https://github.com/scalvert>. However, since after the
first migration from ember-best-practices we cannot rely on the fact that
committer == author of the rule, we'll have to add attribution to all the
existing rules as well. WDYT @michalsnik <https://github.com/michalsnik>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD52d10VYlbatbbfyCP8BaAokrc1Q5Jpks5sQNvugaJpZM4Oc4Op>
.
|
|
@jbandura - my point is we should separate the committer and author, so that we can distinguish between the two. The committer may in fact be the author, or they may not. We can specify the author if the committer != author. Does this make sense? @michalsnik - I'll go ahead and amend the commit for #99 to correctly set the author as @chadhietala. |
|
Actually @michalsnik I think you have to do it. I've not been added as a collaborator on this repo yet, and as such cannot rewrite history. |
|
I'm not a big fun of rewriting history @scalvert.. Feel free to create a separate PR. I'll do the same with the rest of the files anyway. Are you ok with this? :) |
Cover cases such as `const $ = Ember.$`
|
@michalsnik applied your suggestions and added test cases for |
|
LGTM 🚀 |
|
@jbandura Should this be supported? import jQuery from 'jquery';
const $myEl = jQuery('.my-element'); |
Resolves #53.
Description
This PR introduces rule
no-global-jqueryported (with some modifications and added tests) from ember-best-practices plugin (see #53).