initial configuration #1

damfinkel · 2018-09-10T22:47:47Z

Summary

Created a totally custom configuration for react. This configuration will extend from eslint-config-wolox, so I:

Removed all eslint and prettier rules, which apply to Angular and Node too.
Moved all import, react and jsx-a11y rules here, as they are specific to React.
Removed all extensions but eslint-config-wolox, to make the rules explicit and totally custom
Added import and react rules.

Important: This project can only be published after eslint-config-wolox is updated

mvbattan · 2018-09-11T18:44:40Z

index.js

+  plugins: ["react"],
+  extends: ["wolox"],
+  globals: {
+    __DEV__: true


Does this conflict with __DEV__ variable of react-native ?

It shouldn't. It was also in the eslint-config-wolox repo which I understand we were using in RN projects

mvbattan · 2018-09-11T18:47:24Z

index.js

+    "import/extensions": [
+      "error",
+      "never",
+      { js: "never", svg: "always", scss: "always" }


What about other images like png or css ? Shall this be extended by avoiding .jsx as well ?

mvbattan · 2018-09-11T18:48:23Z

index.js

+    ],
+    "react/jsx-curly-spacing": "error",
+    "react/jsx-equals-spacing": "error",
+    "react/jsx-filename-extension": ["error", { extensions: [".js"] }],


Or .jsx is being contemplated here ?

I think it enforces not to use .jsx just .js

I see this rule fine

Yeah, the idea was to avoid .jsx. Do you think we should allow .jsx?

Avoid .jsx please 😇

mvbattan · 2018-09-11T18:51:27Z

index.js

+    "react/no-children-prop": "error",
+    "react/no-danger": "error",
+    "react/no-deprecated": "error",
+    "react/no-did-mount-set-state": "error",


We should allow setState on componentDidMount. This rule was because a rerender needed to be avoided, but in practice and according to React official post, migrating from componentWillMount to DidMount may involve preserving setStates.

I don't see why a setState may be done in componentDidMount. Besides, as you said, it makes an extra render as an initial render happens before componentDidMount and another one will be triggered by this change of state, which is an antipattern. You can set the initial state in constructor, or in getDerivedStateFromProps (which runs once before mounting).

@mvbattan Could you share the link to this docs?

Even so, if there is in fact a use case where you'd want to do it it should have to be an exception rather than a rule. I prefer having to disable the linter for that special case.

There you go:
https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#fetching-external-data
From these docs:

There is a common misconception that fetching in componentWillMount lets you avoid the first empty rendering state.
In practice this was never true because React has always executed render immediately after componentWillMount.
If the data is not available by the time componentWillMount fires, the first render will still show a loading state regardless of where you initiate the fetch.
This is why moving the fetch to componentDidMount has no perceptible effect in the vast majority of cases.

Oh, I see what you're saying now. Sure, setState in a callback is fine. We almost never have to di that, given that all "callbacks" for requests are in the actionCreator. For the very few cases we may have to do this, I think it's better to disable it, and have this rule to avoid setState outside callbacks in componentDidMount. Also, we could just extract the async call to another function so this warning doesn't appear.

Unless you think there might be usual cases where this has to be done.

mvbattan · 2018-09-11T18:54:09Z

index.js

+    "react/no-find-dom-node": "error",
+    "react/no-is-mounted": "error",
+    "react/no-multi-comp": "error",
+    "react/no-render-return-value": "error",


Does this conflicts with React's Portals ?

Here's the argument for this rule https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-render-return-value.md

Regarding ReactDOM.createPortal, I don't think this rule applies to that.

Here's the react docs that support it: https://reactjs.org/docs/react-dom.html#render

ReactDOM.render() currently returns a reference to the root ReactComponent instance. However, using this return value is legacy and should be avoided because future versions of React may render components asynchronously in some cases.

tl;dr: apparently the behaviour of using the result of ReactDOM.render() may be deprecated.

mvbattan · 2018-09-11T18:55:50Z

index.js

+    "react/no-multi-comp": "error",
+    "react/no-render-return-value": "error",
+    "react/no-typos": "error",
+    "react/no-string-refs": "error",


IMHO if possible, we should update this one to only support this:
https://reactjs.org/docs/refs-and-the-dom.html#creating-refs

I've checked literally all react's eslint plugin and there's not rule for that apparently.

mvbattan · 2018-09-11T18:56:48Z

index.js

+    "react/no-render-return-value": "error",
+    "react/no-typos": "error",
+    "react/no-string-refs": "error",
+    "react/no-this-in-sfc": "error",


Is there a way to prevent this accessing at other places, like mapStateToProps or async actions ?

There's an eslint rule to avoid using this in anything that's not a class I think. But I don't think we want that. There may be cases where we want to reference the current object we are in, or whatever this is in certain contexts.

This is the rule: https://eslint.org/docs/rules/no-invalid-this

I think that these cases aren't very common 🤔 In this case, I'd disable the linter rule. In my experience, dealing with this in a mapStateToProps due to a bad copy/paste was painful, specially because didn't have too much experience with this tech

Sorry, I'm confused about which rule we're talking about. Do you like or dislike this rule (react/no-this-in-sfc)? I can't find any harm on having it, you just shouldn't use this in stateless functional components.

On the other hand, I think we can discuss having the no-invalid-this rule. General eslint rules will come from a config repo we'll share with Frontend and Node, and I think we agreed on not having that rule, but we could override it here if we want. I don't have a strong opinion on this, I'm leaning a little bit towards not having the rule because it's not a "bad practise", only something that could be an issue if we don't understand what this really is in each context. The solution would be to understand it better instead of disabling something that could be useful.

But again, I'm still not sure if avoiding possible bugs given that it's not very common having to use this outside a class could be a better tradeoff.

@wfolini @SKOLZ what do you think?

I like this rule. I don't see the reason for not implementing it and doing the opposite, using this in sfc. My vote is a yes

@damfinkel I like this rule but was wondering if this can be extensible to other use cases 😇

no-invalid-this will be set as "error" in eslint-config-wolox

mvbattan · 2018-09-11T18:58:50Z

index.js

+    "react/no-this-in-sfc": "error",
+    "react/no-unescaped-entities": "error",
+    "react/no-unknown-property": "error",
+    "react/no-unsafe": "error",


I'd delete this one, since UNSAFE_ methods may be used in React Native (there is an specific case using Animations or PanResponder, but I don't remember 😇)

I don't know how common that use case is. If it's not that common, I'd prefer keeping the rule and disabling it in that specific case, so in the rest of the app we avoid use of UNSAFE methods.

mvbattan · 2018-09-11T19:02:53Z

index.js

+    "react/no-unsafe": "error",
+    "react/no-unused-prop-types": "error",
+    "react/no-unused-state": "error",
+    "react/no-will-update-set-state": "error",


Also, I'd add no-colors-literals for RN and SASS, and no-inline-styles. What do you think ?

totally agree!

I'll add it, but I haven't actually checked eslint-plugin-react-native. We may have to do that in a next iteration or consider using a different repo for that config which extends from this if it may crash with React

wfolini

Excellent!

damfinkel · 2018-09-18T14:04:07Z

index.js

+    "react/boolean-prop-naming": "error",
+    "react/button-has-type": "error",
+    "react/default-props-match-prop-types": "error",
+    "react/destructuring-assignment": [


@mvbattan @wfolini what do you think about this rule?

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/destructuring-assignment.md

@SKOLZ thinks that it's very annoying to have to do this in every method. While I agree, I also think this rule will give us a lot more consistency that we have today in how we use props/state. The problem is the following:

Good scenario:

render() { const { a, b, c } = this.props; // this is what the rule enforces ... }

Bad scenario:

const foo = () => { const { onlyThingUsed } = this.props; // The rule also enforces this ... }

And there's no way to specify a minimum of properties used to enforce the rule. There are 2 open issues for this, but the mantainers don't want to do it: jsx-eslint/eslint-plugin-react#1731

We could do it ourselves, but for the time being, what do you think? Do we want it or not?

The only 'good think' doing the bad scenario is when we'd like to distinguish what props are being modified at the start of the function.
But, IMHO this is annoying and the 'benefit' previously mentioned shouldn't be called a 'benefit' since functions in general shouldn't be too large.

I honestly found it impractical. Few were the times that it help me. I think the code is more neat but I do not see much advantage, it becomes a little bit annoying

wfolini · 2018-09-19T17:01:20Z

index.js

+    "react/no-render-return-value": "error",
+    "react/no-typos": "error",
+    "react/no-string-refs": "error",
+    "react/no-this-in-sfc": "error",


I like this rule. I don't see the reason for not implementing it and doing the opposite, using this in sfc. My vote is a yes

…sions

damfinkel assigned SKOLZ, mvbattan and wfolini Sep 10, 2018

initial configuration

bd91772

SKOLZ approved these changes Sep 10, 2018

View reviewed changes

mvbattan reviewed Sep 11, 2018

View reviewed changes

mvbattan assigned damfinkel and unassigned mvbattan Sep 11, 2018

wfolini reviewed Sep 13, 2018

View reviewed changes

wfolini removed their assignment Sep 13, 2018

damfinkel commented Sep 18, 2018

View reviewed changes

damfinkel assigned mvbattan and wfolini Sep 18, 2018

mvbattan removed their assignment Sep 18, 2018

damfinkel assigned mvbattan Sep 18, 2018

wfolini approved these changes Sep 19, 2018

View reviewed changes

wfolini removed their assignment Sep 19, 2018

mvbattan approved these changes Sep 19, 2018

View reviewed changes

mvbattan assigned SKOLZ and unassigned SKOLZ and mvbattan Sep 19, 2018

added 2 react-native rules and png and css extensions to import/exten…

6940419

…sions

damfinkel force-pushed the first-version branch from 57925d5 to 6940419 Compare September 23, 2018 23:54

damfinkel merged commit ef6aea3 into master Sep 23, 2018

initial configuration #1

initial configuration #1

Conversation

damfinkel commented Sep 10, 2018 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvbattan Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damfinkel Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damfinkel Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damfinkel Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damfinkel commented Sep 10, 2018 •

edited

Loading

mvbattan Sep 18, 2018 •

edited

Loading

damfinkel Sep 18, 2018 •

edited

Loading

damfinkel Sep 18, 2018 •

edited

Loading

damfinkel Sep 18, 2018 •

edited

Loading