Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update no-global-jquery rule to account for new modules import #186

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Update no-global-jquery rule to account for new modules import #186

merged 2 commits into from
Dec 18, 2017

Conversation

clcuevas
Copy link
Contributor

This PR updates the no-global-jquery rule to not report the $ module when using the new modules import syntax.

account for new modules import syntax
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I believe you have stepped in quick sand with this one.

As I reviewed this PR I notice many issues with state management in the preexisting code that we need to address.

  • ImportDeclaration -- always clobbers importAliasName (previously emberImportAliasName) even if that import has nothing to do with Ember or jquery modules
  • VariableDeclaration -- when an importAliasName (previously emberImportAliasName) is set (which as you can see from above is already in a bad state) we always clobber the destructuredAssignment variable

Some illustrating failing test cases for the above:

  • Using any import other than Ember or jquery breaks the rule:
import Ember from 'ember';
import Foo from 'bar';

Ember.$('.lololol');
  • Destructuring anything after destructuring jQuery will not fail properly:
import Ember from 'ember';

const { $ } = Ember;
const { eq } = Ember.computed;
  • Having a variable declaration for anything other than Ember.$ will not fail properly:
import Ember from 'ember';

const { $ } = Ember;
const wut = 'lolerskates';
$('.whyowhyowhy')

Maybe more scenarios, but I think this comment is already really long 😝

@clcuevas - Would you like to help fix these issues?

@clcuevas
Copy link
Contributor Author

@rwjblue Thanks for looking into this and providing feedback. I'm definitely up for this task. Would you like for me to close this PR and open a new one once I've fixed the above issues/concerns you stated?

@rwjblue
Copy link
Member

rwjblue commented Nov 21, 2017

Totally up to you, I'm happy to iterate with you here or in separate more focused PRs. Whichever seems more comfortable for you...

@clcuevas
Copy link
Contributor Author

Okay, I'll leave this PR open and see where it goes... 😄

@clcuevas
Copy link
Contributor Author

@rwjblue I have something that I've labeled "WIP". I wanted to push it up here just so you can have a look and make sure I'm moving in the right direction. I'm still trying to figure out if there is a better way to perform the checks we need to make for this rule. But, this requires that I read a lot of documentation for ESLint and working with the rules, etc.

@rwjblue
Copy link
Member

rwjblue commented Nov 23, 2017

Yes, that’s looking much better! Thanks for working on it!

I think the next major thing to do is add the various test cases I listed above, and confirm the changes here address them (and adjust if not).

@Turbo87 Turbo87 added the bug label Nov 23, 2017
@clcuevas
Copy link
Contributor Author

@rwjblue I think this is ready to go for a review!

add more test scenarios
@yads
Copy link

yads commented Dec 11, 2017

it would be great to get this merged in since the current default configuration contradicts itself.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing!

This is looking really good, and fixes all of the main state management issues I pointed out earlier.

I'm 👍 on landing, @Turbo87 any objections?

@rwjblue rwjblue merged commit 96c0713 into ember-cli:master Dec 18, 2017
@rwjblue
Copy link
Member

rwjblue commented Dec 18, 2017

Thank you again @clcuevas!

@danwenzel
Copy link
Contributor

FYI: It looks like there's one case that's still failing: #187 (comment)

@clcuevas clcuevas deleted the jquery branch December 28, 2017 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants