-
-
Notifications
You must be signed in to change notification settings - Fork 686
Simplify configuration #17
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
Conversation
Could you please review this PR in your spare time @mysticatea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR!
Mostly looks good to me. I write some points.
.eslintrc.js
Outdated
mocha: true, | ||
}, | ||
extends: [ | ||
"plugin:eslint-plugin/recommended", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue seems to use plugin:vue-libs/recommended
: .eslintrc of vuejs/vue. I think good to follow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true. This might be a good idea 👍
override: | ||
- npm test | ||
- nvm use 6 && npm test | ||
- nvm use 7 && npm test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think good if we use 8 instead of 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add 8 additionally. These tests run pretty fast and we'll be sure it's actually bulletproof.
config/base.js
Outdated
}, | ||
|
||
env: { | ||
browser: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I dislike browser
environment because this environment includes many global variables which has general names: name
, length
, status
, event
, open
, parent
, etc.... It will possibly cause the problems about no-undef
/ no-unused-vars
/ no-shadow
rules.
So I prefer that it defines fewer global variables than browser
environment manually (then I use window.name
instead of name
) (browser.js is my config for frontend. This defines classes and whitelisted variables of browser
environment).
However, I know I'm in the minority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep this simple 'browser' setting for now, at least for v3.0.0-beta to get a better picture about possible issues with this during feedback gathering period. WDYT @mysticatea ?
I addressed your comments @mysticatea. If it's ok I can merge this and release |
Oh, |
package.json
Outdated
}, | ||
"engines": { | ||
"node": ">=4" | ||
}, | ||
"peerDependencies": { | ||
"eslint": "^3.18.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, ESLint 4.0.0 has been released. I'd like to include 4.x in peerDependencies to avoid installation failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. peerDependencies
updated
I also added |
@mysticatea Please approve if you agree with my latest updates and I'll release beta. I removed recommendations for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you so much!
…d-contributing-templates ASA-194: Add templates for bugs, feature requests and PRs
General purpose of this PR is to clean and simplify current code base to get a solid ground for upcoming
v3.0.0-beta
release.Things, that I'm working on in this PR:
update-rules
scriptjsx-uses-vars
rule, that was previously available ineslint-plugin-vue
(+ documentation and tests)I intentionally removed mysticatea's config from
.eslintrc
so that we can make a deliberate decision about preferred styleguide in order to make this plugin's style match other internal projects. I think we should use something more general, likeairbnb-base
. But I'd like to hear your thoughts as well @chrisvfritz @mysticatea @yyx990803