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 eslint-config-airbnb to version 11.0.0 🚀 #1042

Merged
merged 7 commits into from
Sep 14, 2016

Conversation

greenkeeperio-bot
Copy link
Contributor

Hello lovely humans,

eslint-config-airbnb just published its new version 11.0.0.

State Update 🚀
Dependency eslint-config-airbnb
New version 11.0.0
Type devDependency

This version is not covered by your current version range.

Without accepting this pull request your project will work just like it did before. There might be a bunch of new features, fixes and perf improvements that the maintainers worked on for you though.

I recommend you look into these changes and try to get onto the latest version of eslint-config-airbnb.
Given that you have a decent test suite, a passing build is a strong indicator that you can take advantage of these changes by merging the proposed change into your project. Otherwise this branch is a great starting point for you to work on the update.

Do you have any ideas how I could improve these pull requests? Did I report anything you think isn’t right?
Are you unsure about how things are supposed to work?

There is a collection of frequently asked questions and while I’m just a bot, there is a group of people who are happy to teach me new things. Let them know.

Good luck with your project ✨

You rock!

🌴


This pull request was created by greenkeeper.io.

Tired of seeing this sponsor message? ⚡ greenkeeper upgrade

@kumar303
Copy link
Contributor

kumar303 commented Sep 9, 2016

Due to a bug (jsx-eslint/eslint-plugin-react#816) triggered by our use of destructors, we can't specify the real shape of the objects we depend on. This isn't such a big deal to me because static prop type checking has so many flaws already.

@kumar303 kumar303 force-pushed the greenkeeper-eslint-config-airbnb-11.0.0 branch from d27a60a to ce538c3 Compare September 9, 2016 21:23
@kumar303
Copy link
Contributor

kumar303 commented Sep 9, 2016

@mstriemer could you take a look since there are quite a few changes?

@kumar303 kumar303 force-pushed the greenkeeper-eslint-config-airbnb-11.0.0 branch from ce538c3 to 27c90da Compare September 9, 2016 21:40
@@ -5,7 +5,7 @@ import './style.scss';

export default class JsonData extends React.Component {
static propTypes = {
data: PropTypes.object.isRequired,
data: PropTypes.shape({}).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer that we change react/forbid-prop-types to allow object rather than use shape({}) all over. Do we expect that we'll actually define what the shape is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it feels like changing to shape with an empty object is sort of sweeping the issue eslint is detecting under the carpet and if we're going to do that, we might as well relax this rule.

One thing I did find playing around with this is that the detection of shape props is a bit limited so as soon as you start using it you'll find you need to relax one of the unused props rules (to ignore shape props) because the current rules fail to detect the usage of the props as soon as you alias the object (which is a common pattern with destructuring).

Overall, I like the idea of a warning when someone uses object as its good to get into the practice of properly specifying the object shape. However, doing that is only really useful if using shape will provide warnings if the props aren't used or don't match up. What I'm not sure of is what the console will tell us via react's dev warnings vs these lint warnings.

@mstriemer
Copy link
Contributor

Looks good. I'd like to avoid the noise on the shape({}) change. I'd also rather write object than shape({}) and I'm not convinced we care about prop types enough to make proper use of shape().

Is there some advantage to defining the shape that I'm missing?

@mstriemer
Copy link
Contributor

r+wc

// static class methods on React components when they don't use `this`.
// However, it will not warn you if you access `this` in a static method
// accidentally (which would most likely be a mistake).
"class-methods-use-this": "off",
Copy link
Contributor

Choose a reason for hiding this comment

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

I was curious so I looked this up. Static methods in JS have access to this. E.g you can call another static method from a static method, or access other static properties.

But, static methods are only enumerable as properties of the class object. You can't call a static method as a property of an instance (unlike Python which allows both). Because of that the this in a static method is a reference to the object and not ever the instance.

See http://jsbin.com/dubelog/edit?js,console for an example.

It makes sense we don't want to start converting methods that don't use this into static methods because that will probably cause breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I was playing with this too. I can't think of any case where we'd want to make a react component's method static if that method is used by render(). Maybe AirBnB had this rule because they don't use lifecycle react classes? I know stateless components is all the hotness.

@kumar303
Copy link
Contributor

The problem with object is that pretty much anything satisfies that PropType. If we use shape() everywhere then we have to explicitly define what kind of object we accept and theoretically this will catch incompatibilities (especially in refactoring).

Now, enter the real world:

So, yeah, I'll remove shape() for now but I personally would love to add it back in if some of these bugs get fixed. I've found this kind of static analysis to be really helpful when refactoring.

@kumar303 kumar303 force-pushed the greenkeeper-eslint-config-airbnb-11.0.0 branch from c4e8a6a to 630afd0 Compare September 12, 2016 22:03
@@ -22,7 +22,7 @@ class OuterComponent extends Component {

class InnerComponent extends Component {
static propTypes = {
i18n: PropTypes.object.isRequired,
i18n: PropTypes.shape({}).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over shape

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, just got busy. I haven't removed them all yet.

Copy link
Contributor

@kumar303 kumar303 Sep 14, 2016

Choose a reason for hiding this comment

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

oh, hah, I did remove them! And I did indeed forget these, thanks.
owl

@kumar303 kumar303 merged commit 8a2fe85 into master Sep 14, 2016
@kumar303 kumar303 deleted the greenkeeper-eslint-config-airbnb-11.0.0 branch September 14, 2016 21:21
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.

5 participants