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.

Sign up for GitHub

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

Jump to bottom

initial configuration #1

Merged
merged 2 commits into from
Sep 23, 2018
Merged

initial configuration #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd91772
initial configuration
damfinkel Sep 10, 2018
6940419
added 2 react-native rules and png and css extensions to import/exten…
damfinkel Sep 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
"use strict";

module.exports = {
env: {
es6: true,
node: true,
browser: true
},
parser: "babel-eslint",
parserOptions: {
ecmaFeatures: {
experimentalObjectRestSpread: true,
jsx: true
},
sourceType: "module"
},
plugins: ["react", "import", "jsx-a11y", "react-native"],
extends: ["wolox"],
globals: {
__DEV__: true
},
rules: {
// Import
"import/default": "error",
"import/export": "error",
"import/exports-last": "error",
"import/extensions": [
"error",
"never",
{ js: "never", svg: "always", scss: "always", png: "always", css: "always" }
],
"import/first": "error",
"import/named": "error",
"import/namespace": "error",
"import/newline-after-import": "error",
"import/no-absolute-path": "error",
"import/no-extraneous-dependencies": "error",
"import/no-mutable-exports": "error",
"import/no-named-as-default-member": "error",
"import/no-named-default": "error",
"import/no-self-import": "error",
"import/no-unresolved": "error",
"import/no-useless-path-segments": "error",
"import/no-webpack-loader-syntax": "error",
"import/order": ["error", { "newlines-between": "always" }],
"import/prefer-default-export": "off",

// jsx-a11y
"jsx-a11y/anchor-is-valid": "error",

// React
"react/boolean-prop-naming": "error",
"react/button-has-type": "error",
"react/default-props-match-prop-types": "error",
"react/forbid-dom-props": ["error", { forbid: ["style", "id"] }],
"react/forbid-prop-types": "error",
"react/jsx-boolean-value": ["error", "never"],
"react/jsx-child-element-spacing": "error",
"react/jsx-closing-bracket-location": "error",
"react/jsx-closing-tag-location": "error",
"react/jsx-curly-brace-presence": [
"error",
{ props: "never", children: "never" }
],
"react/jsx-curly-spacing": "error",
"react/jsx-equals-spacing": "error",
"react/jsx-filename-extension": ["error", { extensions: [".js"] }],
Copy link

Choose a reason for hiding this comment

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

Or .jsx is being contemplated here ?

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

I see this rule fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Avoid .jsx please 😇

"react/jsx-first-prop-new-line": "error",
"react/jsx-handler-names": "error",
"react/jsx-indent": ["error", 2],
"react/jsx-key": "error",
"react/jsx-max-depth": ["error", { max: 4 }],
"react/jsx-no-bind": "error",
"react/jsx-no-comment-textnodes": "error",
"react/jsx-no-duplicate-props": "error",
"react/jsx-no-target-blank": "error",
"react/jsx-one-expression-per-line": "error",
"react/jsx-pascal-case": "error",
"react/jsx-props-no-multi-spaces": "error",
"react/jsx-sort-default-props": "error",
"react/jsx-tag-spacing": ["error", { beforeClosing: "never" }],
"react/jsx-uses-vars": "error",
"react/jsx-wrap-multilines": [
"error",
{
declaration: "parens-new-line",
assignment: "parens-new-line",
return: "parens-new-line",
arrow: "parens-new-line",
condition: "parens-new-line",
logical: "parens-new-line",
prop: "parens-new-line"
}
],
"react/forbid-foreign-prop-types": "error",
"react/no-access-state-in-setstate": "error",
"react/no-array-index-key": "error",
"react/no-children-prop": "error",
"react/no-danger": "error",
"react/no-deprecated": "error",
"react/no-did-mount-set-state": "error",
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Agree!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

@mvbattan mvbattan Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"react/no-did-update-set-state": "error",
"react/no-direct-mutation-state": "error",
"react/no-find-dom-node": "error",
"react/no-is-mounted": "error",
"react/no-multi-comp": "error",
"react/no-render-return-value": "error",
Copy link

Choose a reason for hiding this comment

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

Does this conflicts with React's Portals ?

Copy link
Contributor Author

@damfinkel damfinkel Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"react/no-typos": "error",
"react/no-string-refs": "error",
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"react/no-this-in-sfc": "error",
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@damfinkel damfinkel Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"react/no-unescaped-entities": "error",
"react/no-unknown-property": "error",
"react/no-unsafe": "error",
Copy link

Choose a reason for hiding this comment

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

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 😇)

Copy link
Contributor Author

@damfinkel damfinkel Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"react/no-unused-prop-types": "error",
"react/no-unused-state": "error",
"react/no-will-update-set-state": "error",
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

totally agree!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Great :atom:

"react/prefer-es6-class": "error",
"react/prefer-stateless-function": "error",
"react/prop-types": [2, { ignore: ["style", "children", "dispatch"] }],
"react/require-default-props": "off",
"react/react-in-jsx-scope": "error",
"react/require-render-return": "error",
"react/self-closing-comp": "error",
"react/sort-prop-types": [
"error",
{
ignoreCase: true,
callbacksLast: true,
requiredFirst: true,
sortShapeProp: true
}
],
"react/style-prop-object": "error",
"react/void-dom-elements-no-children": "error"

// React Native
"no-colors-literals": "error",
"no-inline-styles": "error"
},
settings: {
"import/resolver": {
node: {
extensions: [".js", ".android.js", ".ios.js"]
}
}
}
};
5 changes: 5 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 33 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"name": "@wolox/eslint-config-react",
"version": "1.0.0",
"description": "Wolox's ESLint configuration for React projects ",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"repository": {
"type": "git",
"url": "git+https://github.com/Wolox/eslint-config-wolox-react.git"
},
"keywords": [
"ESLint",
"React",
"Wolox"
],
"author": "Damián Finkelstein <dfinkelstein@wolox.com.ar>",
"license": "MIT",
"bugs": {
"url": "https://github.com/Wolox/eslint-config-wolox-react/issues"
},
"homepage": "https://github.com/Wolox/eslint-config-wolox-react#readme",
"peerDependencies": {
"eslint": "^5.6.0",
"eslint-config-wolox": "2.0.0",
"eslint-plugin-flowtype": "^2.50.0",
"eslint-plugin-import": "^2.14.0",
"eslint-plugin-jsx-a11y": "^6.1.1",
"eslint-plugin-react": "^7.11.1",
"eslint-plugin-react-native": "^3.3.0"
}
}