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

Add new "no-old-shims" rule #90

Merged
merged 4 commits into from
Jul 10, 2017
Merged

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jul 7, 2017

This PR resolves a major part of #86

Example from the documentation:

// GOOD
import Component from '@ember/component'
import EmberObject, { computed } from '@ember/object'
import Service, { inject } from '@ember/service'

// BAD
import Component from 'ember-component';
import EmberObject from 'ember-object';
import computed from 'ember-computed';
import Service from 'ember-service';
import inject from 'ember-service/inject';

The rule also supports eslint --fix

/cc @michalsnik @rwjblue @cibernox

Copy link
Member

@michalsnik michalsnik left a comment

Choose a reason for hiding this comment

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

Looks really good @Turbo87, thanks for working on this!

@@ -0,0 +1,79 @@
'use strict';

const oldShimsData = require('ember-rfc176-data/old-shims.json');
Copy link
Member

Choose a reason for hiding this comment

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

Nice one!

let localName;
let importedName;
if (specifier.type === 'ImportDefaultSpecifier') {
localName = specifier.local.name;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we can move localName outside of this condition and use const instead:

const localName = specifier.local.name;

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


return {
ImportDeclaration(node) {
const source = node.source.value;
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about packageName or moduleName instead of source? For a person that looks at this code for the first time it will probably be more understandable. It's just a suggestion though

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@michalsnik
Copy link
Member

michalsnik commented Jul 10, 2017

One more thing @Turbo87 I think we should add this rule in https://github.com/netguru/eslint-plugin-ember/blob/master/config/base.js though it will require us to bump a major version of this plugin then instead of a minor. Or maybe you don't think that this rule is that crucial at this moment and we can postpone adding it to default configuration?

@Turbo87
Copy link
Member Author

Turbo87 commented Jul 10, 2017

I'd recommend to try this out for a bit in a minor release and then enable it later if everything works as expected. Since the rule is only useful if you use Babel 6 I'd could also lead to a few false positives if it's enabled by default.

@michalsnik
Copy link
Member

Alright, that makes perfect sense to me.

@michalsnik michalsnik merged commit a864868 into ember-cli:master Jul 10, 2017
@michalsnik
Copy link
Member

Released as v3.6.0 🚀 Thanks @Turbo87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants